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

check if version is missing from config.yml #223

Merged
merged 1 commit into from
Aug 25, 2020

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Aug 23, 2020

this is a common mistake to add a new version to the conandata.yml, but forget to add it to the config.yml.
as result, CI doesn't build the newly added version, and we don't know if it actually passed the CI.
it's very easy to miss during the review, so I've decided to add an automatic check for human error.

NOTE: it's not needed to add the opposite case - version is added to the config.yml but not to the conandata.yml, as it would fail anyway during the attempt to fetch sources.

docs: conan-io/conan-center-index#2640

@SSE4
Copy link
Contributor Author

SSE4 commented Aug 24, 2020 via email

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.

assertNotIn 🤦 ...too early in the morning...

@danimtb
Copy link
Member

danimtb commented Aug 24, 2020

I am not sure about having this check in the hook, as the config.yml file is something specific to conan-center-index. Maybe this is a check we should do in the pipeline itself. wdyt @jgsogo?

@SSE4
Copy link
Contributor Author

SSE4 commented Aug 24, 2020

@danimtb but this is conan-center hook, right? anyway, if there is no config.yml, the check will not run, so it's harmless

@jgsogo
Copy link
Contributor

jgsogo commented Aug 24, 2020

I agree that hooks shouldn't use config.yml because it is a file that is strange to Conan, in that sense, it would be better to add this check (and any other involving config.yml to the CI), but it is really easy to implement it here, these are conan-center hooks and it is useful (in the future, if we add this check to the CI we can remove it from here).

versions_conandata = conandata_yml['sources'].keys()
versions_config = config_yml['versions'].keys()

for version in versions_conandata:
Copy link
Member

Choose a reason for hiding this comment

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

The opposite also can occur. I can remember one or two cases where the user created a config.yml with more versions but the conandata.yml had only one, like preparing for the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but didn't it fail anyway? because there are no sources in conandata, so source method should have failed

Copy link
Contributor

Choose a reason for hiding this comment

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

We can add the opposite check in the future (even for this same hook).

@Croydon
Copy link
Contributor

Croydon commented Aug 24, 2020

conan-center hooks are already strongly CCI specific anyway, e.g. we check if the homepage is set to CCI

And for the sake of transparency and open source I would actually suggest to implement even more into the hooks =)

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.

5 participants