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

Fix for when args.path is not used #90

Merged
merged 4 commits into from
May 22, 2024
Merged

Fix for when args.path is not used #90

merged 4 commits into from
May 22, 2024

Conversation

michxymi
Copy link
Contributor

@michxymi michxymi commented Nov 14, 2023

With the current implementation of the CCI export_all_versions command you can either pass a config file, a path or a name.

There's currently a bug when not using a path argument. When either the config file or a name is parsed args.path is still used to locate the recipes directory. However because no path has been parsed as an arg it is undefined (i.e None).

This PR adds a check condition to check if args.path is undefined and if it is, it uses the cwd/recipes path, to comply with the expectation that this script should run from the root of a CCI file tree. When a path is indeed requested and args.path is not undefined, its value is used.

@michxymi
Copy link
Contributor Author

@uilianries Decided to make my first contribution :)

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

@michxymi Thank you for your first contribution! 🎉

Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Thanks a lot! :)

We're delaying the merge until we do some behind the scenes work on the repo itself, will ping you when ready

@michxymi
Copy link
Contributor Author

Thanks both. I should have started from this but I also added some unit tests for all the possible args.

@CLAassistant
Copy link

CLAassistant commented Nov 15, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

New tests!! ❤️

Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

This got lost in the sea of other work - thanks to @danimtb for the ping that this was still open :)

@AbrilRBS AbrilRBS merged commit e66d286 into conan-io:main May 22, 2024
2 checks passed
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