-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add support for autopublish and autodistribute #199
Conversation
441fafc
to
4b7c8fc
Compare
If this looks good, I'll add in support for pulp_rpm. |
6729be8
to
abee46a
Compare
update_options = [ | ||
click.option("--base-path"), | ||
click.option("--publication"), | ||
click.option("--repository", callback=_repository_callback), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This all looks good to me. And i believe the nightly is failing, because those changes will land there tomorrow!? |
87f80e1
to
16304c5
Compare
4c68c49
to
24b3086
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One uncertainty wrt the changelog.
click.option("--gpgcheck", type=click.Choice(("0", "1"))), | ||
click.option("--repo-gpgcheck", type=click.Choice(("0", "1"))), | ||
click.option("--sqlite-metadata/--no-sqlite-metadata", default=None), | ||
click.option("--autopublish/--no-autopublish", default=None), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of completes the repository options, not necessarily related to auto publish.
Do we want to reflect that in the changelog?
update_options = [ | ||
click.option("--description"), | ||
click.option("--remote", callback=_remote_callback), | ||
click.option("--manifest"), | ||
click.option("--autopublish/--no-autopublish", default=None), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the default should simply be --no-autopublish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an update_option. So if not specified, we should def not set (and then update) this option to False
.
I think having None
here, keeps it from being touched on update calls, and on create calls i'd rather let the server decide on the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I believe I misunderstood what it does then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it should default to --no-autopublish
. The dilemma is that this will cause autopublish
to always be set in the requests which won't work for older versions of pulp_file/pulp_rpm and we support 5 versions back.
I could either:
- Check the version of pulp_file and if it's less than 1.7.0, silently strip out the autopublish key/value from the request.
- Keep the default as None which won't set autopublish in requests unless the user explicitly sets
--autopublish
or--no-autopublish
. I could add a comment for us to change the default once we drop support for <1.7.0.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welp, ignore my comment. I am too slow.
), | ||
click.option("--gpgcheck", type=click.Choice(("0", "1"))), | ||
click.option("--repo-gpgcheck", type=click.Choice(("0", "1"))), | ||
click.option("--sqlite-metadata/--no-sqlite-metadata", default=None), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
fixes #155