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
remove inotify-tools dep and AoLogs scaffolded for ansible based-operator #2852
remove inotify-tools dep and AoLogs scaffolded for ansible based-operator #2852
Conversation
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.
/lgtm
CHANGELOG.md
Outdated
|
||
### Removed | ||
|
||
- The `inotify-tools` as dependency of Ansible based-operator images which was deprecated and it will no longer scaffold the `/bin/ao-logs` which was using it to print the Ansible logs in the side-car since the side-car ansible container was removed in the previous versions. [#2852](https://github.com/operator-framework/operator-sdk/pull/2852) |
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.
Revert this CHANGELOG change and replace with a fragment file in changelog/fragments
f87689d
to
6622ebb
Compare
New changes are detected. LGTM label has been removed. |
6622ebb
to
dd4b293
Compare
changelog/fragments/2852-pr.yaml
Outdated
# entries is a list of entries to include in | ||
# release notes and/or the migration guide | ||
entries: | ||
- description: The `inotify-tools` as a dependency of Ansible based-operator images which was deprecated and it will no longer scaffold the `/bin/ao-logs` which was using it to print the Ansible logs in the side-car since the side-car ansible container was removed in the previous versions. [#2852](https://github.com/operator-framework/operator-sdk/pull/2852) |
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.
Remove the PR link. That will be generated and appended automatically.
changelog/fragments/2852-pr.yaml
Outdated
# Migration can be defined to automatically add a section to | ||
# the migration guide. This is required for breaking changes. | ||
migration: | ||
header: The `inotify-tools` dependency was removed and `/bin/ao-logs` are no longer generated for Ansible based-operators |
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.
Nit: not a huge deal, but this is probably a little long for a header and might not render super well in the migration guide.
Not sure what the best general approach is for this. We could add a header character limit that we check when validating as part of the sanity test. WDYT?
changelog/fragments/2852-pr.yaml
Outdated
migration: | ||
header: The `inotify-tools` dependency was removed and `/bin/ao-logs` are no longer generated for Ansible based-operators | ||
body: > | ||
The `inotify-tools` as a dependency of Ansible based-operator images which was deprecated in the previous versions was removed. This dependency was used to output the Ansible logs in a side-car container called `ansible`, which has no longer been scaffolded by default. In this way, if you have any customization using this dependency be aware that it is no longer in the Ansible based-operator image and then, feel free to remove this file `/bin/ao-logs` for your projects since it has no longer usage. For further information also see the section `Remove Ansible container sidecar` in this guide. |
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.
For further information also see the section
Remove Ansible container sidecar
in this guide.
We're splitting out the migration guide into separate files for each release, so this text will end up in v0.18.0.md
. I think we're also planning to split the existing migration guide into individual release migration guides, so linking over to that file might not be the best idea either.
Is there a PR related to removing the ansible container sidecar we could link to instead?
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.
For the users know the steps, I think that the best would be the migration guide section and not the PR. I will try to figure out something here.
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.
If this isn't urgent, I can try to throw together a PR to split out the existing version upgrade guide, get that merged, and then you can link to the new one?
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.
It has not the links. I mean it has just the information and then it was updated based on this comment.
Could you please check and let us know if you are ok with?
dd4b293
to
8462c47
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.
LGTM
Description of the change:
inotify-tools
as a dependency of Ansible based-operator images which was deprecated and it will no longer scaffold the/bin/ao-logs
which was using it to print the Ansible logs in the side-car since the side-car ansible container was removed in the previous versions. #2852Motivation for the change:
PR to ensure that all required logs will be print in the operator container:
Add event stats output to the operator logs for Ansible based-operators. #2580
PR to remove the Ansible sidecar container: Deprecated
inotify-tools
on Ansible based-operator images and remove Ansible sidecar container. #2586