Skip to content
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

Refactor build-release role tests.yml #432

Merged

Conversation

gotmax23
Copy link
Contributor

@Ompragash inspired me to look into how ansible is actually released, and I noticed some opportunities for improvement. Hopefully, this is helpful. At least, it has provided me with a distraction from my other work :).

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! Would you mind adding a changelog fragment? Thanks.

Comment on lines 6 to 9
antsibull_pip_bin: "{{ antsibull_ansible_venv }}/bin/pip"
antsibull_pip_pkgs: "{{ _pip_pkgs['packages'][antsibull_pip_bin] }}"
ansible_version_pypi: "{{ antsibull_pip_pkgs['ansible'][0]['version'] }}"
_installed_ansible_core: "{{ antsibull_pip_pkgs['ansible-core'][0]['version'] }}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about always using the prefix antsibull_ansible_venv for everything that's in antsibull's ansible venv?

Suggested change
antsibull_pip_bin: "{{ antsibull_ansible_venv }}/bin/pip"
antsibull_pip_pkgs: "{{ _pip_pkgs['packages'][antsibull_pip_bin] }}"
ansible_version_pypi: "{{ antsibull_pip_pkgs['ansible'][0]['version'] }}"
_installed_ansible_core: "{{ antsibull_pip_pkgs['ansible-core'][0]['version'] }}"
antsibull_ansible_venv_pip_bin: "{{ antsibull_ansible_venv }}/bin/pip"
antsibull_ansible_venv_pip_pkgs: "{{ _pip_pkgs['packages'][antsibull_pip_bin] }}"
antsibull_ansible_venv_installed_ansible_version: "{{ antsibull_pip_pkgs['ansible'][0]['version'] }}"
antsibull_ansible_venv_installed_ansible_core_version: "{{ antsibull_pip_pkgs['ansible-core'][0]['version'] }}"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe antsibull_venv? I dislike the long variable names.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine for me. @dmsimard what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 028b35f. I'm thinking it would make sense to completely remove the venv prefix, but I'll defer to you and @dmsimard.

roles/build-release/tasks/tests.yaml Outdated Show resolved Hide resolved
@gotmax23
Copy link
Contributor Author

Would you mind adding a changelog fragment?

I've added a changelog fragment. I've never written an ansible changelog fragment before, so let me know if it needs any changes :).

@gotmax23 gotmax23 force-pushed the build_release_test_refactor branch from db7de56 to 028b35f Compare June 24, 2022 21:07
@gotmax23 gotmax23 force-pushed the build_release_test_refactor branch 2 times, most recently from 0c431fd to 9eff261 Compare June 25, 2022 20:55
Copy link
Member

@Ompragash Ompragash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gotmax23 Thanks for looking into this so quickly!
Suggesting minor changes to replace the left out short names with FQCN

roles/build-release/tasks/tests.yaml Show resolved Hide resolved
roles/build-release/tasks/tests.yaml Outdated Show resolved Hide resolved
roles/build-release/tasks/tests.yaml Outdated Show resolved Hide resolved
roles/build-release/tasks/tests.yaml Outdated Show resolved Hide resolved
gotmax23 and others added 3 commits June 27, 2022 12:54
Signed-off-by: Maxwell G <gotmax@e.email>
Signed-off-by: Maxwell G <gotmax@e.email>

Co-authored-by: Felix Fontein <felix@fontein.de>
Signed-off-by: Maxwell G <gotmax@e.email>
@gotmax23 gotmax23 force-pushed the build_release_test_refactor branch from 9eff261 to 70cfc21 Compare June 27, 2022 17:54
Signed-off-by: Maxwell G <gotmax@e.email>
Signed-off-by: Maxwell G <gotmax@e.email>
@gotmax23 gotmax23 force-pushed the build_release_test_refactor branch from 70cfc21 to f3a9eaf Compare July 4, 2022 16:58
@felixfontein felixfontein merged commit 468273f into ansible-community:main Jul 4, 2022
@felixfontein
Copy link
Collaborator

@gotmax23 thanks for contributing this!
@Ompragash thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants