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

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

Open
wants to merge 24 commits into
base: devel
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
ab35165
Adds intial workflow for Porting Guide PR
anweshadas Jul 8, 2023
8288761
Fixes the directory for the ansible documentation checkout
anweshadas Jul 8, 2023
fcb1258
Fixes the directory for the new branch
anweshadas Jul 8, 2023
1b634de
Fixes the EOF error with qouting
anweshadas Jul 8, 2023
1167701
Add steps to check what is there inside a directory
anweshadas Jul 8, 2023
7a367da
Edits the tar command
anweshadas Jul 8, 2023
a684ca6
Fixes ansible versioning for Porting Guide
anweshadas Jul 8, 2023
c7ae2d3
Fixes tar command for debuging
anweshadas Jul 8, 2023
c873ac3
Fixes directory for PR
anweshadas Jul 8, 2023
52096c5
Fixes the names of the steps
anweshadas Jul 8, 2023
edc88cf
Updates to the correct barnch
anweshadas Jul 14, 2023
d9206c7
Fixes changes based on the review feedback
anweshadas Aug 3, 2023
f5c5813
Cleans git and github steps for better security
anweshadas Dec 4, 2023
8588946
Edits to get Porting guide from ansible-build-data git repo
anweshadas Dec 19, 2023
aea1067
Checks out correct repo
anweshadas Dec 19, 2023
e1bfd19
Edits to retrive major version from user input
anweshadas Mar 25, 2024
dc77ee1
Removes redundant quote from PR text
anweshadas Mar 28, 2024
8f9cd1b
Reformts abd general cleanup
anweshadas Mar 28, 2024
97133cd
Edits to working-directory
anweshadas Mar 28, 2024
68ed0ee
Updates based on feedback
anweshadas May 14, 2024
8eeeac5
Update .github/workflows/docsbuild-release.yaml
anweshadas May 15, 2024
1ada5a3
release porting guide workflow
oraNod May 16, 2024
02478f4
use a reusable workflow
oraNod May 16, 2024
695f1a4
use plain git cmds instead of gha
oraNod May 17, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions .github/workflows/release-porting-guide.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
---
name: Ansible porting guide
on:
workflow_dispatch:
inputs:
ansible-version:
description: Release version. For example, 11.1.0
required: true
ansible-major-version:
description: Major version. For example, 11
required: true

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

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?

Copy link
Member

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

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

runs-on: ubuntu-latest
env:
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
Comment on lines +18 to +21
Copy link
Collaborator

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

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

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

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.

Suggested change
wget ${{ env.package-url }}/${{ env.ansible-tarball }}
wget "${PACKAGE_URL}/${ANSIBLE_TARBALL}"

&&
Copy link
Collaborator

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.

Copy link
Member

@webknjaz webknjaz Jun 28, 2024

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.

tar -xvf ${{ env.ansible-tarball }}

- name: Create an artifact with the RST file
uses: actions/upload-artifact@v4
with:
name: ansible-porting-guide
path: ${{ env.extracted-path }}/${{ env.porting-guide-rst }}
retention-days: 7

create-docs-pr:
name: Create a docs PR${{ '' }} # Nest jobs under the same sidebar category
Copy link
Member

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:

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

needs: upload-porting-guide
uses: ./.github/workflows/reusable-porting-guide.yml
with:
repository: ansible/ansible-documentation
path: ansible-documentation
ansible-version: ${{ inputs.ansible-version }}
ansible-major-version: ${{ inputs.ansible-major-version }}

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

Choose a reason for hiding this comment

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

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

needs: upload-porting-guide
uses: ./.github/workflows/reusable-porting-guide.yml
Copy link
Collaborator

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?

with:
repository: ansible-community/ansible-build-data
path: ansible-build-data
ansible-version: ${{ inputs.ansible-version }}
ansible-major-version: ${{ inputs.ansible-major-version }}
96 changes: 96 additions & 0 deletions .github/workflows/reusable-porting-guide.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
---
name: Create porting guide PR

"on":
workflow_call:
inputs:
repository:
type: string
description: The repository to check out.
required: true
path:
type: string
description: The path for the repository.
required: true
ansible-version:
type: string
description: Ansible release version.
required: true
ansible-major-version:
type: string
description: Ansible major version.
required: true

env:
Copy link
Member

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

git-branch: porting-guide-${{ inputs.ansible-version }}
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: >-
Copy link
Member

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:

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.

##### SUMMARY

Add the Ansible "${{ inputs.ansible-major-version }}" porting guide.
Comment on lines +28 to +32
Copy link
Collaborator

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.

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.


##### ISSUE TYPE

- Docs Pull Request

##### COMPONENT NAME

porting_guide_${{ inputs.ansible-major-version }}.rst

jobs:
create-pr:
runs-on: ubuntu-latest
steps:
Copy link
Member

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.

Suggested change
steps:
defaults:
working-directory: ${{ inputs.path }}
steps:

- name: Check out the repository
uses: actions/checkout@v4
with:
repository: ${{ inputs.repository }}
path: ${{ inputs.path }}

- name: Download the artifact with the RST file
uses: actions/download-artifact@v4
with:
name: ansible-porting-guide
path: ${{ inputs.path }}

- 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 }}
Comment on lines +58 to +66
Copy link
Member

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

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


- name: Set up git
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"
Copy link
Collaborator

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.

Copy link
Member

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

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

- name: Add the porting guide
run: git add . --all --force
Copy link
Member

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.

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

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

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.

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

- name: Push to the repo
run: git push origin "${{ env.git-branch }}"
working-directory: ${{ inputs.path }}

- name: Create the porting guide PR
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 }}
Comment on lines +90 to +96
Copy link
Member

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?