-
Notifications
You must be signed in to change notification settings - Fork 138
Sync variables of test-operator role from defaults/main.yml in README.md automatically #3327
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
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
48da28a
to
8cc7aea
Compare
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/9891cc2d5b67440bba00da71a8abbdda ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 05m 57s |
I would say the commit message should not be "Testing my script to update the README.md file to sync variables of tesat-operator role from defaults/main.yml" but closer to the message of the merge request |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
…esat-operator role from defaults/main.yml Ticket: OSPRH-19423
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
@sauragar: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed 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.
Thank you for the PR! In my opinion the main issue is that we lose parameter descriptions. Please focus on that part, other comments are just suggestions on improving the code.
- name: tempest | ||
type: tempest | ||
cifmw_test_operator_fail_on_test_failure: true | ||
cifmw_test_operator_temp_var: test |
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.
When you are ready to merge this please remove all the test variables.
# Please refer to https://openstack-k8s-operators.github.io/test-operator/guide.html#executing-tempest-tests | ||
cifmw_test_operator_tempest_debug: false | ||
cifmw_test_operator_tempest_temp_var: "{{ stage_vars_dict.cifmw_test_operator_tempest_name }}-{{ _stage_vars.name }}" |
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.
Not sure if this variable is needed, but if it's test please remove it later as well.
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.
Please make sure that the script is just changing the defaults and the format stays the same as it is now. If I understand it correctly, now all the text explaining the variables is gone. A README without variable explanations is not very useful.
Additionally, please keep the markdown the same -> * variable
: (Type) text Default value: value
return cat | ||
# Check keyword in variable name | ||
lowered = key.lower() | ||
for keyword, cat in [("tempest", "Tempest"), |
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.
Can't this be done a bit more simple? Is it not possible to use lowered version everywhere until you want to create the comment, e.g. Horizontest Variables and make the first letter capital in that moment? :D
Also I would keep the name the same, e.g. Horizontest specific parameters.
for cat, items in categorized.items(): | ||
if not items: | ||
continue | ||
md_sections.append(f"### {cat} Variables\n") |
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.
Based on my comment above I would change this part where the markdown sections are created.
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.
Please make sure the Generic
section comes first. I would also not add the header if we know it comes first -> it will look the same as the current README.
def update_readme(defaults): | ||
readme = readme_file.read_text() | ||
|
||
start_marker = "<!-- START DEFAULTS -->" |
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 think this could also be named start parameters
because it will not only be the defaults, but also parameters descriptions and types.
Testing my script to update the README.md file to sync variables of test-operator role from defaults/main.yml in README.md automatically via script
Ticket: OSPRH-19423