Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update GitHub Info #6

Merged
merged 11 commits into from
Feb 1, 2019
Merged

Update GitHub Info #6

merged 11 commits into from
Feb 1, 2019

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Oct 20, 2018

similar to the Bintray Update plug-in

uses GitHub API https://developer.github.com/v3/

  • update description
  • update homepage (requires conan 1.9.0)
  • update topics/tags (requires conan 1.9.0)

depends on:

@Croydon
Copy link
Contributor

Croydon commented Nov 20, 2018

homepage was added in the same pull request and discussed in the same issue as the topics attribute

@jgsogo
Copy link
Contributor

jgsogo commented Jan 2, 2019

We have added Travis and AppVeyor CI to this repo and an example test for the attribute_checker hook. We need to merge master into this PR (to get CI) and then implement some tests to check that the hook is working (it should be easy to mock requests.* function, but ping me if you need help).

Thanks!

@SSE4
Copy link
Contributor Author

SSE4 commented Jan 9, 2019

I am going to update and test this one and then I will ping you, thanks

@jgsogo
Copy link
Contributor

jgsogo commented Jan 9, 2019

Have a look at the discussion and the kind of testing we are talking about in #2, feedback is very welcome.

@jgsogo
Copy link
Contributor

jgsogo commented Jan 28, 2019

@SSE4, please move this hook to the hooks folder, and document it in the README.md. We are willing to merge all the hooks and start promoting this feature.

Thanks

@SSE4
Copy link
Contributor Author

SSE4 commented Jan 29, 2019

@jgsogo updated!

hooks/github-updater.py Outdated Show resolved Hide resolved
hooks/github-updater.py Outdated Show resolved Hide resolved
@jgsogo
Copy link
Contributor

jgsogo commented Jan 29, 2019

Although I'm not a great fan of env-variables I think that another one is needed to allow the user to restrict the version/branch that will modify the Github properties, as they are unique for all the repository.

This env variable should be optional, and I'm not sure if it should be targetting the branch name or the recipe reference,... what do you think?

@SSE4
Copy link
Contributor Author

SSE4 commented Jan 29, 2019

unfortunately these properties aren't per branch, but per repository, so how to do in this case?

@jgsogo
Copy link
Contributor

jgsogo commented Jan 29, 2019

If I have a CONAN_GITHUB_HOOK_MAIN_BRANCH=master I can get the branch name from the repo, compare it to the value of that variable and make the request or not.

Or I can have a CONAN_GITHUB_HOOK_MAIN_REF_PATTERN=*/*@conan/stable, get the reference and act according to it.

Or maybe I should check if current branch is the default one in the remote repo (no need for an env variable).


Maybe the option I like most is the last one...

hooks/github-updater.py Outdated Show resolved Hide resolved
@SSE4
Copy link
Contributor Author

SSE4 commented Jan 31, 2019

@jgsogo this might be useful, but I have some doubts:

  • it adds dependency on Git executable availability
  • it adds an additional complexity to the hook
  • it requires package folder to be a git repository, which is not necessary a case (e.g. I may have downloaded an archive from GitHub)

I suppose better and cleaner approach is to just check conan channel if it's stable or release, WDYT?

@jgsogo
Copy link
Contributor

jgsogo commented Jan 31, 2019

@SSE4, is this comment related to this PR?

@SSE4
Copy link
Contributor Author

SSE4 commented Jan 31, 2019

@jgsogo yes, its about your suggestion for CONAN_GITHUB_HOOK_MAIN_BRANCH

@jgsogo
Copy link
Contributor

jgsogo commented Jan 31, 2019

Ok, I forgot my previous comment, I was just checking the sources 😓

Yes, the suggestion about checking against a local repo is stupid, forget it... nevertheless, I would like to know more opinions about the second one (or something similar), two ways to implement it:

  • CONAN_GITHUB_HOOK_MAIN_REF_PATTERN=/@conan/stable
  • Query Github to know which one is the default branch (I think it should always be the last stable version, isn't it?) and update that information only if it matches the version/channel of the Conan reference.

@SSE4
Copy link
Contributor Author

SSE4 commented Jan 31, 2019

the last one is the most logical and simple for me (query GitHub default branch)

@jgsogo
Copy link
Contributor

jgsogo commented Jan 31, 2019

Also, it doesn't require another env variable... and I really think it is useful to keep the information updated between the repo and the last version of the recipe.

@SSE4
Copy link
Contributor Author

SSE4 commented Jan 31, 2019

@jgsogo I've added required check for the default branch

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's start using it!

@danimtb danimtb merged commit 5d35335 into conan-io:master Feb 1, 2019
danimtb pushed a commit to danimtb/hooks that referenced this pull request May 20, 2019
Skip CMAKE MODULES/PC-FILES for cmake recipe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants