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

fix(.github/workflows): update all the workflows #610

Merged
merged 5 commits into from
Jan 25, 2024

Conversation

Wabri
Copy link
Member

@Wabri Wabri commented Jan 17, 2024

for reference: #421

Closes #420

@Wabri Wabri added the enhancement New feature or request label Jan 17, 2024
@Wabri Wabri force-pushed the feature/improve_workflow branch from c54fd18 to bcba6ac Compare January 17, 2024 12:32
@Wabri Wabri self-assigned this Jan 17, 2024
@kksat
Copy link
Contributor

kksat commented Jan 17, 2024

maybe as we are on this topic also unify python versions used. In some workflows python 3.9 is used, in some python 3.12

@berndfinger berndfinger self-requested a review January 17, 2024 14:27
@berndfinger
Copy link
Member

maybe as we are on this topic also unify python versions used. In some workflows python 3.9 is used, in some python 3.12

Right. We will soon move to the latest version also in the GitHub actions of the remaining roles, now that we know that it is working for the collection and for the role sap_vm_preconfigure.

@kksat
Copy link
Contributor

kksat commented Jan 18, 2024

@Wabri Thank you

@Wabri
Copy link
Member Author

Wabri commented Jan 18, 2024

I can also create a reusable workflow in order to simplify the maintainability of the workflows, so that in case of python or ansible updates we only modify in one place.

This is the doc:
https://docs.github.com/en/actions/using-workflows/reusing-workflows

@berndfinger
Copy link
Member

@Wabri - Feel free to share your proposal. If you also could incorporate the updated versions for ansible-lint and the other packages (in the ansible-lint workflow for the collection), then we would not need another PR for those.

One problem with the role level workflows we discovered recently was that some of the ansible-lint checks depend on the correct detection of the role name (e.g. var-naming[no-role-prefix]). I think running a collection-level workflow for each PR for one or a few roles would slow down our development and consume more CPU resources than necessary, so maybe there is a way to run ansible-lint just for one role but in a way that it is aware of the role name?

We also need to discuss (and decide on) the latest vs. fixed actions topic as mentioned by @rainerleber in #421 (comment) .

@Wabri
Copy link
Member Author

Wabri commented Jan 18, 2024

I've create the reusable workflow, with this method maintaining the workflows will be better!

ps: the actions are failing because the job ansible-lint call a reusable in the main, but at this moment that workflow doesn't exists for now => uses: sap-linuxlab/community.sap_install/.github/workflows/ansible-lint-sap_reusable_workflow.yml@main

@rainerleber
Copy link
Contributor

@Wabri thank you for the improvement. I love the reusable approach.

@sean-freeman
Copy link
Member

I like the current path, but we should really lock down exactly what versions of Python, Ansible Core, Ansible Lint and explicitly handle the dependencies (instead of installing Ansible Community Edition)?

I would suggest:

N.B. It is relatively redundant to install Ansible Community Edition with pip3 install ansible==7.5.0, when this will automatically install the associated Ansible Core (negating the requirement for pip3 install ansible-core==2.14.z).

I like the proposed method, using a locked Python, Ansible Core, Ansible Lint combination during the GH Action and specifically append any dependencies:

  • Python Packages
  • Ansible Collections e.g. community.general or ansible.posix

Fixing this GH Action has been a long-time tech debt imho, and is why I suggested #420 to version lock us to a specific Ansible Core + compatible Ansible Lint version - by using the GitHub Action ansible-lint-action](https://github.com/ansible/ansible-lint-action) with a version lock to a specific release of Ansible Creator Execution Environment (a docker image containing locked versions of ansible-core and ansible-lint with their depedencies).

I'm good to close the referenced GH Issue, if this PR is merged. They achieve the same outcome (version lock on everything) via different methods, the method proposed in this PR keeps us fully in control.

@rainerleber
Copy link
Contributor

to reduce the maintaining as I proposed we should enable the renovate bot to automatically create pr's if a new version of the used actions are available as well the python modules like ansible

@Wabri
Copy link
Member Author

Wabri commented Jan 18, 2024

@sean-freeman I added the Closes #420 to the description in order to close when the PR is merged.

@rainerleber I read something in the docs, but do you have any idea to implement that workflow? do you think adding this:

- name: Renovate Bot GitHub Action
  uses: renovatebot/[email protected]

will be enough? I saw in the ansible section that there is a default configuration https://docs.renovatebot.com/modules/manager/ansible/ and it works also in our case!

@rainerleber
Copy link
Contributor

we should more likely add it from the marketplace then we don't need it to integrate it as a workflow and the renovate will run continuous in the background

@sean-freeman
Copy link
Member

sean-freeman commented Jan 18, 2024

@rainerleber Do we want renovatebot? In my experience ansible-lint version increments can suddenly mark everything as failing and distract from high value coding - end up forever chasing every linting improvement that alters perfectly functional code?

@rainerleber
Copy link
Contributor

The RenovateBot only creates a pr to suggest a new version of the used actions. After the pr is created the actions will run with the new proposed version only in the pr and we are able to decide if we want to merge. It could make our live easier i think

@Wabri
Copy link
Member Author

Wabri commented Jan 20, 2024

In order to use the RenovateBot we need to choose between run the instance of Renovate in a self-hosting env or subscribe to the mend.io and let them take care of the hosting (https://github.com/marketplace/renovate)

@sean-freeman
Copy link
Member

sean-freeman commented Jan 20, 2024

@Wabri No funds for subscribing to mend, and I have no desire to create self-hosted GH Actions - appends immediate effort we don't have time for and long-term maintenance (security patches, audits and compliance etc). I suggest continuing without RenovateBot and re-visit the topic in 12 months.

@kksat
Copy link
Contributor

kksat commented Jan 21, 2024

Mendi subscription is free of charge

“ This app is free to install for both public and private repositories. Service is provided complimentary of Mend (formerly known as WhiteSource) and no paid plan is required.”

https://github.com/apps/renovate

@Wabri
Copy link
Member Author

Wabri commented Jan 22, 2024

Yes, it's free of charge, but I don't know if we have some kind of policy about subscription.

@Wabri Wabri changed the base branch from main to dev January 23, 2024 16:32
@berndfinger
Copy link
Member

I suggest the following to proceed with this PR:

  • @Wabri - modify .github/workflows/ansible-lint-sap_reusable_workflow.yml to use the following versions (which we already successfully use in the ansible-lint workflow for the collection and for one of the roles:
pip3 install ansible==9.1.0
pip3 install ansible-compat==4.1.10
pip3 install ansible-core==2.16.2
pip3 install ansible-lint==6.22.1

@sean-freeman Advantage over using the suggested versions in #610 (comment): When using later versions now, we could maybe extend the time between this version bump and the next one. Remember: These packages are only used for these .ansible-lint GitHub actions. We may use different versions in other actions with a different scope.

Once this is done, we can merge this PR so the future GitHub actions will use these versions, and we have just one file where we maintain them.

  • @rainerleber or @Wabri - Create a new PR with your proposal about a bot which creates a PR after a new version of underlying software is detected.

@Wabri
Copy link
Member Author

Wabri commented Jan 24, 2024

@berndfinger done the changes you requested

About the new pr with the proposal, should we open a new issue? or is better a new idea thread on the discussion board?

@berndfinger
Copy link
Member

@berndfinger done the changes you requested

Great, thanks!

About the new pr with the proposal, should we open a new issue? or is better a new idea thread on the discussion board?

Yes, good idea to continue the discussion of the dependency/renovate bots in the discussion section. We probably can use the already existing thread #421 for this.

Copy link
Member

@berndfinger berndfinger left a comment

Choose a reason for hiding this comment

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

LGTM!

with:
python-version: '3.12'

- name: Install test dependencies
Copy link
Member

Choose a reason for hiding this comment

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

Let's use the versions of [this task](https://github.com/sap-linuxlab/community.sap_install/blob/5ee5cbe030ad152edfe96567155812f9abcdc25f/.github/workflows/ansible-lint.yml#L26) until the end of the 2024 or until there is a major new ansible-lint requirement for Ansible-Galaxy or Automation Hub before that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we open a new issue or discussion to have a memo about this?

@berndfinger
Copy link
Member

berndfinger commented Jan 24, 2024

ansible-lint just asked me to bump to 6.22.2. With that, I now have:

ansible 9.1.0 (unchanged)
ansible-lint 6.22.2 (newer version)
ansible-core 2.16.2 (unchanged)
ansible-compat 4.1.11 (newer version)
The result of an ansible-lint run for the collection and the roles did not bring up any new violations, so I now suggest to use these later versions in this PR.

@Wabri - Can you please bump the versions for the two affected packages again?

P.S. I am not saying that we shouldn't do a version bump if ansible-lint 6.22.2 had brought up additional errors, but we should have had a second look into the new errors first.

Copy link
Contributor

@rainerleber rainerleber left a comment

Choose a reason for hiding this comment

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

lgtm

@berndfinger berndfinger merged commit 65326d2 into sap-linuxlab:dev Jan 25, 2024
2 checks passed
@Wabri Wabri deleted the feature/improve_workflow branch January 25, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

collection: ansible-lint GH Action Workflow improvements
5 participants