-
Notifications
You must be signed in to change notification settings - Fork 2
Unset all upper constraints files #72
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
Summary of ChangesHello @MoteHue, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request removes the hardcoded default OpenStack upper constraints file from various Ansible roles. By unsetting these variables, the roles will no longer implicitly enforce a specific set of Python dependency versions, providing greater flexibility in managing package installations. This change allows for more dynamic dependency resolution or requires explicit constraint definition by the user, adapting to different deployment environments or OpenStack releases. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request removes the hardcoded default upper constraints file URL from various Ansible roles, which is a good step towards making the roles more flexible. My review focuses on improving the consistency of how these unset variables are defined. I've suggested explicitly setting them to an empty string ("") to align with existing patterns in the codebase, which enhances clarity and maintainability.
| os_container_clusters_templates: [] | ||
| # Upper constraints file for installation of python dependencies. | ||
| os_container_clusters_upper_constraints_file: https://releases.openstack.org/constraints/upper/2024.1 | ||
| os_container_clusters_upper_constraints_file: |
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 consistency with other variables in the collection (e.g., os_images_dib_upper_constraints_file in roles/os_images/defaults/main.yml), it would be better to explicitly set this to an empty string ("") rather than null (which is what an empty value becomes in YAML). This makes the intent clearer and maintains a consistent style across the roles.
os_container_clusters_upper_constraints_file: ""|
|
||
| # Upper constraints file for installation of Python dependencies. | ||
| os_deploy_templates_upper_constraints_file: https://releases.openstack.org/constraints/upper/2024.1 | ||
| os_deploy_templates_upper_constraints_file: |
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 consistency with other variables in the collection (e.g., os_images_dib_upper_constraints_file in roles/os_images/defaults/main.yml), it would be better to explicitly set this to an empty string ("") rather than null (which is what an empty value becomes in YAML). This makes the intent clearer and maintains a consistent style across the roles.
os_deploy_templates_upper_constraints_file: ""|
|
||
| # Use Caracal upper constraints | ||
| os_images_upper_constraints_file: https://releases.openstack.org/constraints/upper/2024.1 | ||
| os_images_upper_constraints_file: |
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 consistency with os_images_dib_upper_constraints_file in this same file, it would be better to explicitly set this to an empty string ("") rather than null (which is what an empty value becomes in YAML). This makes the intent clearer and maintains a consistent style.
os_images_upper_constraints_file: ""|
|
||
| # Upper constraints file for installation of Python dependencies. | ||
| os_networks_upper_constraints_file: https://releases.openstack.org/constraints/upper/2024.1 | ||
| os_networks_upper_constraints_file: |
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 consistency with other variables in the collection (e.g., os_images_dib_upper_constraints_file in roles/os_images/defaults/main.yml), it would be better to explicitly set this to an empty string ("") rather than null (which is what an empty value becomes in YAML). This makes the intent clearer and maintains a consistent style across the roles.
os_networks_upper_constraints_file: ""| os_openstackclient_version: | ||
| # Upper constraints file for installation of openstackclient. | ||
| os_openstackclient_upper_constraints_file: https://releases.openstack.org/constraints/upper/2024.1 | ||
| os_openstackclient_upper_constraints_file: |
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 consistency with other variables in the collection (e.g., os_images_dib_upper_constraints_file in roles/os_images/defaults/main.yml), it would be better to explicitly set this to an empty string ("") rather than null (which is what an empty value becomes in YAML). This makes the intent clearer and maintains a consistent style across the roles.
os_openstackclient_upper_constraints_file: ""| os_openstacksdk_version: | ||
| # Upper constraints file for installation of openstacksdk. | ||
| os_openstacksdk_upper_constraints_file: https://releases.openstack.org/constraints/upper/2024.1 | ||
| os_openstacksdk_upper_constraints_file: |
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 consistency with other variables in the collection (e.g., os_images_dib_upper_constraints_file in roles/os_images/defaults/main.yml), it would be better to explicitly set this to an empty string ("") rather than null (which is what an empty value becomes in YAML). This makes the intent clearer and maintains a consistent style across the roles.
os_openstacksdk_upper_constraints_file: ""| # 'quotas': Optional dict mapping quota names to their values. | ||
| os_projects: [] # noqa var-naming[no-role-prefix] | ||
| os_projects_upper_constraints_file: https://releases.openstack.org/constraints/upper/2024.1 | ||
| os_projects_upper_constraints_file: |
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 consistency with other variables in the collection (e.g., os_images_dib_upper_constraints_file in roles/os_images/defaults/main.yml), it would be better to explicitly set this to an empty string ("") rather than null (which is what an empty value becomes in YAML). This makes the intent clearer and maintains a consistent style across the roles.
os_projects_upper_constraints_file: ""| os_ratings_venv: | ||
| # Upper constraints file for installation of Python dependencies. | ||
| os_ratings_upper_constraints_file: https://releases.openstack.org/constraints/upper/2024.1 | ||
| os_ratings_upper_constraints_file: |
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 consistency with other variables in the collection (e.g., os_images_dib_upper_constraints_file in roles/os_images/defaults/main.yml), it would be better to explicitly set this to an empty string ("") rather than null (which is what an empty value becomes in YAML). This makes the intent clearer and maintains a consistent style across the roles.
os_ratings_upper_constraints_file: ""|
|
||
| # A file or URL containing Python upper constraints. | ||
| os_volumes_upper_constraints_file: https://releases.openstack.org/constraints/upper/2024.1 | ||
| os_volumes_upper_constraints_file: |
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 consistency with other variables in the collection (e.g., os_images_dib_upper_constraints_file in roles/os_images/defaults/main.yml), it would be better to explicitly set this to an empty string ("") rather than null (which is what an empty value becomes in YAML). This makes the intent clearer and maintains a consistent style across the roles.
os_volumes_upper_constraints_file: ""|
My personal feeling is that there should some version locking, ideally with a CI job that bumps that version when there is a new release, otherwise you are going to get unrepeatable behavior - works on one day and broken on next. This is especially true since we refresh the venv on every run. I imagine using upper constraints was an attempt at doing version locking. It might better just to the constraints from the latest stable release? |
We have a PR for 2025.1: #71 |
|
Generally I would be in favor as it breaks every once in a while when the upper constraints branch goes into unmaintained, but I am concerned that just pulling from whatever is the latest and greatest upstream would yield more breakages. |
No description provided.