-
Notifications
You must be signed in to change notification settings - Fork 523
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
Initial (GHA) Workflow for creating the Porting Guide #121
base: devel
Are you sure you want to change the base?
Conversation
Retriggering CI. @anweshadas, what's the current status of this? |
@anweshadas Hey, I just pushed a commit to your branch. I thought that might be easier than adding a load of review comments here. Anyway I hope the changes are OK. Feel free to take them or leave them. I did use yamllint to check the syntax and tried to make things a bit more descriptive in the workflow. For instance, I renamed the job from "build" to "porting-guide" and also changed the name of the workflow file. Hopefully I have the right flow here too. My understanding is that this workflow is intended to extract the porting guide from the package tarball and then add it to the I was also thinking we could break this into three separate jobs:
Not sure what folks might think about that so I didn't go that far in my changes. Happy to take a stab at it if that would be useful. Hope these changes help! Cheers. |
Also it seemed unnecessary to me to have a step that uses sed to extract the major version and thought it could just be passed as an input. Guess we could change it back. |
Thinking about it some more and I think we could break the proposed jobs two and three into a reusable workflow. That should reduce duplication. The main difference between the two jobs is the path where the RST file goes. We could pass that as an input to the reusable workflow. Having the separate jobs should also mean that they run in parallel. |
Just pushed another commit with the reusable workflow. I think it's a bit nicer because the jobs run more independently. Here's what it looks like in the sidebar view: I found that using an action for the PR means we can reduce some of the steps: https://github.com/marketplace/actions/create-pull-request However it ran into a permissions issue (always). I guess we'll need to work through that. It should be fine for this repo (ansible-documentation) because the settings allow for actions to create PRs. For the build-data repo I suppose we'll need a token or something. Another nice thing about using the PR action means we'll be able to specify things like labels, assignees, and reviewers. |
Another commit to add back the plain git steps instead of using that PR action. I guess we can decide which method to use. Just thought maybe I shouldn't change things too much. @anweshadas feel free to either squash that last commit or drop it. |
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.
The changes look good to me. Thank you @oraNod for working on it.
@felixfontein can you please have a look at it? |
|
||
jobs: | ||
upload-porting-guide: | ||
name: Extract the porting guide${{ '' }} # Nest jobs under the same sidebar category |
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.
Why is that ${{ '' }}
needed, and what does the comment wants to say? Does the comment refer to the templating, or to something else?
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.
@felixfontein it's unnecessary in this specific instance. But in general, it's a hack I invented to fight how poorly GHA is rendering matrixes of reusable workflows in the sidebar by default (with multiple collapsed sections for each of the jobs within a matrix).
name: Extract the porting guide${{ '' }} # Nest jobs under the same sidebar category | |
name: Extract the porting guide |
package-url: https://files.pythonhosted.org/packages/source/a/ansible | ||
ansible-tarball: ansible-${{ inputs.ansible-version }}.tar.gz | ||
extracted-path: ansible-${{ inputs.ansible-version }} | ||
porting-guide-rst: porting_guide_${{ inputs.ansible-major-version }}.rst |
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.
Environment variables should always be written upper case and use _
instead of -
.
package-url: https://files.pythonhosted.org/packages/source/a/ansible | |
ansible-tarball: ansible-${{ inputs.ansible-version }}.tar.gz | |
extracted-path: ansible-${{ inputs.ansible-version }} | |
porting-guide-rst: porting_guide_${{ inputs.ansible-major-version }}.rst | |
package-url: https://files.pythonhosted.org/packages/source/a/ansible | |
ansible-tarball: ansible-${{ inputs.ansible-version }}.tar.gz | |
extracted-path: ansible-${{ inputs.ansible-version }} | |
porting-guide-rst: porting_guide_${{ inputs.ansible-major-version }}.rst |
package-url: https://files.pythonhosted.org/packages/source/a/ansible | |
ansible-tarball: ansible-${{ inputs.ansible-version }}.tar.gz | |
extracted-path: ansible-${{ inputs.ansible-version }} | |
porting-guide-rst: porting_guide_${{ inputs.ansible-major-version }}.rst | |
PACKAGE_URL: https://files.pythonhosted.org/packages/source/a/ansible | |
ANSIBLE_LATEST: ansible-${{ inputs.ansible-version }}.tar.gz | |
EXTRACTED_PATH: ansible-${{ inputs.ansible-version }} | |
PORTING_GUIDE_RST: porting_guide_${{ inputs.ansible-major-version }}.rst |
steps: | ||
- name: Fetch and unpack the package tarball | ||
run: >- | ||
wget ${{ env.package-url }}/${{ env.ansible-tarball }} |
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.
In shell scripts, always use ${ENV_VAR}
instead of ${{ env.ENV_VAR }}
, and always quote.
wget ${{ env.package-url }}/${{ env.ansible-tarball }} | |
wget "${PACKAGE_URL}/${ANSIBLE_TARBALL}" |
- name: Fetch and unpack the package tarball | ||
run: >- | ||
wget ${{ env.package-url }}/${{ env.ansible-tarball }} | ||
&& |
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.
Why use >-
and &&
, and not simply use |
and write the commands in different lines?
If a command fails, the whole step fails anyway.
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.
If a command fails, the whole step fails anyway.
@felixfontein that's actually not universally true. There are corner cases (mostly for Windows runners IIRC) where the second command would still get executed, and its return code would be used for the step status. To fight this, either boilerplate like set -eEuo pipefail
should be put in front of the script or something like shell: bash -eEuo {0}
should be used.
create-build-data-pr: | ||
name: Create a build-data PR${{ '' }} # Nest jobs under the same sidebar category | ||
needs: upload-porting-guide | ||
uses: ./.github/workflows/reusable-porting-guide.yml |
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.
What is this step for? Why does a PR in ansible-build-data needs to be created?
Add the Ansible community "${{ inputs.ansible-version }}" porting guide | ||
pr-body-message: >- | ||
##### SUMMARY | ||
|
||
Add the Ansible "${{ inputs.ansible-major-version }}" porting guide. |
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.
There should be no quotes used here. We do not have an Ansible "10" porting guide
, we have an Ansible 10 porting guide
.
Add the Ansible community "${{ inputs.ansible-version }}" porting guide | |
pr-body-message: >- | |
##### SUMMARY | |
Add the Ansible "${{ inputs.ansible-major-version }}" porting guide. | |
Add the Ansible community "${{ inputs.ansible-version }}" porting guide | |
pr-body-message: >- | |
##### SUMMARY | |
Add the Ansible "${{ inputs.ansible-major-version }}" porting guide. |
Add the Ansible community "${{ inputs.ansible-version }}" porting guide | |
pr-body-message: >- | |
##### SUMMARY | |
Add the Ansible "${{ inputs.ansible-major-version }}" porting guide. | |
Add the Ansible community ${{ inputs.ansible-version }} porting guide | |
pr-body-message: >- | |
##### SUMMARY | |
Add the Ansible ${{ inputs.ansible-major-version }} porting guide. |
run: | | ||
git checkout -b "${{ env.git-branch }}" | ||
git config --global user.name "${{ github.actor }}" | ||
git config --global user.email "${{ github.actor }}@users.noreply.github.com" |
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.
Put things that aren't in an environment variable into one for this step, and use envrionment variables. Otherweise there is no sane and safe way to use these in run
scripts.
- name: Commit the porting guide | ||
run: >- | ||
git diff-index --quiet HEAD || | ||
git commit -m "${{ env.ci-commit-message }}" |
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.
Especially this use is extremely dangerous. Never use GHA interpolation inside run
scripts if you don't know that the content is extremely simple and limited. The commit message is very far from that.
porting-guide-rst: porting_guide_${{ inputs.ansible-major-version }}.rst | ||
ci-commit-message: >- | ||
Add the Ansible community "${{ inputs.ansible-version }}" porting guide | ||
pr-body-message: >- |
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.
Use the pipe syntax, since >
merges all the lines into one:
pr-body-message: >- | |
pr-body-message: |- |
Tip
See https://yaml-multiline.info to experiment and see how different multiline string block scalars are rendered.
working-directory: ${{ inputs.path }} | ||
|
||
- name: Add the porting guide | ||
run: git add . --all --force |
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.
Don't use --force
unless you want to add things that are gitignored.
I'd also recommend being more granular and committing what's expected to exist + adding an assertion that no uncommitted files left.
description: Ansible major version. | ||
required: true | ||
|
||
env: |
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.
Technically, environment variables should be referenceable in Bash and therefore shouldn't contain dashes. But keep using dashes in the GHA-only vars (like inputs/outputs and job/step IDs).
retention-days: 7 | ||
|
||
create-docs-pr: | ||
name: Create a docs PR${{ '' }} # Nest jobs under the same sidebar category |
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.
It is only necessary to use the ${{ '' }}
hack in job matrices with reusable workflows:
name: Create a docs PR${{ '' }} # Nest jobs under the same sidebar category | |
name: Create a docs PR |
ansible-major-version: ${{ inputs.ansible-major-version }} | ||
|
||
create-build-data-pr: | ||
name: Create a build-data PR${{ '' }} # Nest jobs under the same sidebar category |
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.
name: Create a build-data PR${{ '' }} # Nest jobs under the same sidebar category | |
name: Create a build-data PR |
- name: Copy the RST file to the docs repo | ||
if: ${{ inputs.repository == 'ansible/ansible-documentation' }} | ||
run: cp -v ${{ env.porting-guide-rst }} docs/docsite/rst/porting_guides/ | ||
working-directory: ${{ inputs.path }} | ||
|
||
- name: Copy the RST file to the build-data repo | ||
if: ${{ inputs.repository == 'ansible-community/ansible-build-data' }} | ||
run: cp -v ${{ env.porting-guide-rst }} ${{ inputs.ansible-major-version }} | ||
working-directory: ${{ inputs.path }} |
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.
These steps are almost identical. Perhaps, it's best to collapse these into a single step, with the target computed based on inputs..
- name: Copy the RST file to the docs repo | |
if: ${{ inputs.repository == 'ansible/ansible-documentation' }} | |
run: cp -v ${{ env.porting-guide-rst }} docs/docsite/rst/porting_guides/ | |
working-directory: ${{ inputs.path }} | |
- name: Copy the RST file to the build-data repo | |
if: ${{ inputs.repository == 'ansible-community/ansible-build-data' }} | |
run: cp -v ${{ env.porting-guide-rst }} ${{ inputs.ansible-major-version }} | |
working-directory: ${{ inputs.path }} | |
- name: Copy the RST file to the ${{ inputs.repository }} repo | |
run: >- | |
cp -v | |
'${{ env.porting-guide-rst }}' | |
'${{ | |
inputs.repository == 'ansible/ansible-documentation' | |
&& 'docs/docsite/rst/porting_guides/' | |
|| inputs.ansible-major-version | |
}}' | |
working-directory: ${{ inputs.path }} |
run: | | ||
git checkout -b "${{ env.git-branch }}" | ||
git config --global user.name "${{ github.actor }}" | ||
git config --global user.email "${{ github.actor }}@users.noreply.github.com" |
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.
a proper email should contain the account id too FYI
jobs: | ||
create-pr: | ||
runs-on: ubuntu-latest | ||
steps: |
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.
Since all the steps need the same working dir, you can just change the default.
steps: | |
defaults: | |
working-directory: ${{ inputs.path }} | |
steps: |
run: >- | ||
gh pr create | ||
--base test | ||
--head "publish-${{ inputs.ansible-version }}" | ||
--title "${{ env.ci-commit-message }}" | ||
--body "${{ env.pr-body-message }}" | ||
working-directory: ${{ inputs.path }} |
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.
The working directory is irrelevant here, FYI.
How about implementing something like pytest-dev/pytest#12502 to improve the UX?
@anweshadas Is there any update on this PR? It's over a year old now. Should we just close it as won't do? @felixfontein From your side do you think there is still a need for this workflow? Thanks |
@anweshadas Please let us know if you plan to continue working on this issue or if we should close it. Thanks. |
@oraNod I will start working on this. please do not close this. |
Depends on ansible-community/ansible-build-data#265