-
Notifications
You must be signed in to change notification settings - Fork 31
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
Split antsibull-build's single subcommand into prepare and rebuild-single; deprecate multiple #341
Split antsibull-build's single subcommand into prepare and rebuild-single; deprecate multiple #341
Conversation
Need more time to parse the PR, let me get back to you on that |
68e6893
to
618e1f9
Compare
I decided to change the |
I like this and it helps move in the direction I'd eventually picture things to be like 👍 At a high level: I believe the contents of ansible-build-data should not exclusively be artifacts of a build only pushed after the fact. We should be able to It's OK if the collection versions drift a little until the day of the release -- I figure we can automate collection version updates by re-running prepare and propose a change if there are new versions available. This would allow us to see problems ahead of time rather than on the day of release. I will test the PR and report back. |
I've tested it against a hypothetical 5.1.0 build and the good news is that it works: https://demo.recordsansible.org/playbooks/3221.html The prepare command is only poking at the versions: https://demo.recordsansible.org/results/690056.html In the prepare command there's a message that I don't remember seeing before:
The code LGTM at first glance but I am also not intimately familiar with the antsibull internals. There may be an opportunity to simplify further in the future -- for example, while out of scope, I'm not sure if we need |
618e1f9
to
f138756
Compare
That message was probably already there before, but now it's removed. For that I needed to bump the antsibull-changelog requirement a bit (from 0.7 to 0.10; current release is 0.12 so that should be ok).
I think we should remove |
When creating that PR I noticed that I would have to re-create parts of this PR (splitting up some function), so I decided to simply integrate the deprecation here :) |
…changelog and porting guide RSTs.
571984f
to
81f5efc
Compare
Co-authored-by: David Moreau Simard <moi@dmsimard.com>
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.
Works for me, tested locally. Code looks fine too. Thanks @felixfontein !
@dmsimard thanks a lot for testing and reviewing! |
One thing to determine is whether
prepare
should download the collections already and build changelog and porting guide, as well as updating the changelog.yaml.I would definitely keep updating the changelog.yaml, but the other things are debatable. @dmsimard what do you think?