Initial (GHA) Workflow for creating the Porting Guide #121

anweshadas · 2023-07-14T18:57:12Z

Depends on ansible-community/ansible-build-data#265

.github/workflows/docsbuild-release.yaml

gotmax23 · 2023-10-26T02:48:41Z

Retriggering CI. @anweshadas, what's the current status of this?

oraNod · 2024-05-16T11:51:37Z

@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 ansible-documentation and ansible-build-data repositories. It looked like there were some missing steps and parameters in the original workflow so I've tried to fix that and get the correct order of steps.

I was also thinking we could break this into three separate jobs:

Job one: Extract the RST file from the package tarball and upload it as a workflow artifact.
Job two: Download the RST file artifact and add it to the ansible-documentation repo.
Job three: Download the RST file artifact and add it to the ansible-build-data repo.

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.

oraNod · 2024-05-16T11:56:30Z

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.

oraNod · 2024-05-16T12:35:33Z

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.

oraNod · 2024-05-16T14:50:03Z

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.

oraNod · 2024-05-17T08:59:05Z

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.

anweshadas

The changes look good to me. Thank you @oraNod for working on it.

anweshadas · 2024-05-23T15:15:50Z

@felixfontein can you please have a look at it?

felixfontein · 2024-06-05T19:18:42Z

.github/workflows/release-porting-guide.yml

+
+jobs:
+  upload-porting-guide:
+    name: Extract the porting guide${{ '' }}  # Nest jobs under the same sidebar category


Why is that ${{ '' }} needed, and what does the comment wants to say? Does the comment refer to the templating, or to something else?

@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).

Suggested change

name: Extract the porting guide${{ '' }} # Nest jobs under the same sidebar category

name: Extract the porting guide

felixfontein · 2024-06-05T19:19:28Z

.github/workflows/release-porting-guide.yml

+      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


Environment variables should always be written upper case and use _ instead of -.

Suggested change

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

Suggested change

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

felixfontein · 2024-06-05T19:21:06Z

.github/workflows/release-porting-guide.yml

+    steps:
+    - name: Fetch and unpack the package tarball
+      run: >-
+        wget ${{ env.package-url }}/${{ env.ansible-tarball }}


In shell scripts, always use ${ENV_VAR} instead of ${{ env.ENV_VAR }}, and always quote.

Suggested change

wget ${{ env.package-url }}/${{ env.ansible-tarball }}

wget "${PACKAGE_URL}/${ANSIBLE_TARBALL}"

felixfontein · 2024-06-05T19:21:44Z

.github/workflows/release-porting-guide.yml

+    - name: Fetch and unpack the package tarball
+      run: >-
+        wget ${{ env.package-url }}/${{ env.ansible-tarball }}
+        &&


Why use >- and &&, and not simply use | and write the commands in different lines?

If a command fails, the whole step fails anyway.

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.

felixfontein · 2024-06-05T19:24:04Z

.github/workflows/release-porting-guide.yml

+  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


What is this step for? Why does a PR in ansible-build-data needs to be created?

felixfontein · 2024-06-05T19:25:32Z

.github/workflows/reusable-porting-guide.yml

+    Add the Ansible community "${{ inputs.ansible-version }}" porting guide
+  pr-body-message: >-
+    ##### SUMMARY
+
+    Add the Ansible "${{ inputs.ansible-major-version }}" porting guide.


There should be no quotes used here. We do not have an Ansible "10" porting guide, we have an Ansible 10 porting guide.

Suggested change

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.

Suggested change

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.

felixfontein · 2024-06-05T19:26:36Z

.github/workflows/reusable-porting-guide.yml

+      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"


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.

felixfontein · 2024-06-05T19:27:42Z

.github/workflows/reusable-porting-guide.yml

+    - name: Commit the porting guide
+      run: >-
+        git diff-index --quiet HEAD ||
+        git commit -m "${{ env.ci-commit-message }}"


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.

webknjaz · 2024-06-28T22:06:03Z

.github/workflows/reusable-porting-guide.yml

+  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: >-


Use the pipe syntax, since > merges all the lines into one:

Suggested change

pr-body-message: >-

pr-body-message: |-

Tip
See https://yaml-multiline.info to experiment and see how different multiline string block scalars are rendered.

webknjaz · 2024-06-28T22:08:03Z

.github/workflows/reusable-porting-guide.yml

+      working-directory: ${{ inputs.path }}
+
+    - name: Add the porting guide
+      run: git add . --all --force


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.

webknjaz · 2024-06-28T22:09:34Z

.github/workflows/reusable-porting-guide.yml

+        description: Ansible major version.
+        required: true
+
+env:


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).

webknjaz · 2024-06-28T22:11:23Z

.github/workflows/release-porting-guide.yml

+        retention-days: 7
+
+  create-docs-pr:
+    name: Create a docs PR${{ '' }}  # Nest jobs under the same sidebar category


It is only necessary to use the ${{ '' }} hack in job matrices with reusable workflows:

Suggested change

name: Create a docs PR${{ '' }} # Nest jobs under the same sidebar category

name: Create a docs PR

webknjaz · 2024-06-28T22:11:31Z

.github/workflows/release-porting-guide.yml

+      ansible-major-version: ${{ inputs.ansible-major-version }}
+
+  create-build-data-pr:
+    name: Create a build-data PR${{ '' }}  # Nest jobs under the same sidebar category


Suggested change

name: Create a build-data PR${{ '' }} # Nest jobs under the same sidebar category

name: Create a build-data PR

webknjaz · 2024-06-28T22:21:47Z

.github/workflows/reusable-porting-guide.yml

+    - 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 }}


These steps are almost identical. Perhaps, it's best to collapse these into a single step, with the target computed based on inputs..

Suggested change

- 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 }}

webknjaz · 2024-06-28T22:23:12Z

.github/workflows/reusable-porting-guide.yml

+      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"


a proper email should contain the account id too FYI

webknjaz · 2024-06-28T22:24:44Z

.github/workflows/reusable-porting-guide.yml

+jobs:
+  create-pr:
+    runs-on: ubuntu-latest
+    steps:


Since all the steps need the same working dir, you can just change the default.

Suggested change

steps:

defaults:

working-directory: ${{ inputs.path }}

steps:

webknjaz · 2024-06-28T22:30:58Z

.github/workflows/reusable-porting-guide.yml

+      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 }}


The working directory is irrelevant here, FYI.

How about implementing something like pytest-dev/pytest#12502 to improve the UX?

oraNod · 2024-08-20T13:58:35Z

@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

oraNod · 2024-09-12T11:10:05Z

@anweshadas Please let us know if you plan to continue working on this issue or if we should close it. Thanks.

anweshadas · 2024-09-16T15:44:13Z

@oraNod I will start working on this. please do not close this.

anweshadas marked this pull request as draft July 14, 2023 18:57

anweshadas mentioned this pull request Jul 14, 2023

Ansible Community Package Release via Github Actions ansible-community/community-topics#256

Closed