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

Github actions improvements #6108

Closed
wants to merge 9 commits into from
Closed

Conversation

FritzHoing
Copy link
Contributor

@FritzHoing FritzHoing commented Jun 17, 2024

This pull-request improves the existing github-workflows:

  • enhances modularity by encapsulating a common operation into a reusable action for easier maintenance
  • avoids duplicating the same logic across multiple jobs

📚 Documentation preview 📚: https://volto--6108.org.readthedocs.build/

@mister-roboto
Copy link

@FritzHoing you need to sign the Plone Contributor Agreement to merge this pull request.

Learn about the Plone Contributor Agreement: https://plone.org/foundation/contributors-agreement

Copy link

netlify bot commented Jun 17, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 440da91
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/6677cd43e41cee0008e5eb05

@FritzHoing FritzHoing changed the title ci: make use of github actions Github actions improvements Jun 17, 2024
@stevepiercy
Copy link
Collaborator

@FritzHoing this is a super useful contribution, but we can't accept it until you sign the Plone Contributor Agreement. This PR should also have a change log entry. Would you please take the necessary steps? Thank you!

@mister-roboto
Copy link

@FritzHoing you need to sign the Plone Contributor Agreement to merge this pull request.

Learn about the Plone Contributor Agreement: https://plone.org/foundation/contributors-agreement

@FritzHoing
Copy link
Contributor Author

Thank you very much for your feedback! I signed the Contributor Agreement on monday already and just need to wait for the approval. The changelog entry is now added in the hope that the type is set correct for ci improvements.

@sneridagh
Copy link
Member

@FritzHoing looks great! Thanks a lot for your contribution!

/cc @ericof

@sneridagh
Copy link
Member

@FritzHoing I just allowed the tests to run, could you please take a look? It seems something is missing.

@FritzHoing FritzHoing marked this pull request as draft June 20, 2024 07:45
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

This looks good, but tests are failing. I'll update it against main and see if that fixes it.

packages/volto/news/6108.internal Outdated Show resolved Hide resolved
.github/actions/pre_test_setup/action.yml Outdated Show resolved Hide resolved
.github/workflows/acceptance.yml Show resolved Hide resolved
.github/workflows/acceptance.yml Show resolved Hide resolved
.github/workflows/acceptance.yml Show resolved Hide resolved
.github/workflows/acceptance.yml Show resolved Hide resolved
.github/workflows/acceptance.yml Show resolved Hide resolved
.github/workflows/acceptance.yml Show resolved Hide resolved
.github/workflows/acceptance.yml Show resolved Hide resolved
.github/workflows/acceptance.yml Show resolved Hide resolved
.github/workflows/acceptance.yml Show resolved Hide resolved
@ichim-david ichim-david marked this pull request as ready for review June 23, 2024 07:23
Copy link
Sponsor Member

@ichim-david ichim-david left a comment

Choose a reason for hiding this comment

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

I've commited the changes necessary to get the actions to work, tested and working.
I actually added a new pull request with the changes from this pull request plus my fixes but then I did the extra work to get commit suggestions which can commit to the parent as such I will close #6120

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

A couple questions, and some cleanup.

key: ${{ runner.os }}-pnpm-store-${{ hashFiles('**/pnpm-lock.yaml') }}
restore-keys: |
${{ runner.os }}-pnpm-store-
- uses: actions/checkout@v4
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is duplicated at line 14.

Suggested change
- uses: actions/checkout@v4

node-version: ${{ inputs.node-version }}

- name: Enable corepack
shell: bash
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary now? It wasn't there before. It makes sense to specify the shell when you need something that the shell provides, but not to just run a binary, as in this context.

restore-keys: |
${{ runner.os }}-pnpm-store-

- name: Install dependencies
Copy link
Collaborator

Choose a reason for hiding this comment

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

Install dependencies was previously after Cache Cypress Binary. Does that matter, or affect performance?

@@ -0,0 +1 @@
Improved the existing GitHub workflows by encapsulating a common operation into a reusable action for easier maintenance. @FritzHoing
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Improved the existing GitHub workflows by encapsulating a common operation into a reusable action for easier maintenance. @FritzHoing
Improved the existing GitHub workflows by encapsulating a common operation into a reusable action for easier maintenance. @FritzHoing, @ichim-david


- name: Install Cypress if not in cache
if: steps.cache-cypress-binary.outputs.cache-hit != 'true'
shell: bash
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another possibly unnecessary shell: bash?

@ichim-david
Copy link
Sponsor Member

I made a branch out of the changes native to the volto repo in this pull request #6120
making it easier for me to handle change requests

@ichim-david
Copy link
Sponsor Member

Closing this pull request in favor of #6120 since having the pull request within Volto repo makes it easier to make changes as needed.

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

6 participants