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

Rely on gh run download to extract the artifacts #3591

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Jan 17, 2025

What changes are you introducing?

Turns out gh already unpacks the artifact so the unzip steps are no longer needed. This also means we need multiple indivual commands because they need to be extract to different directories.

Why are you introducing these changes? (Explanation, links to references, issues, etc.)

#3588 (comment)

Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)

I promise I'm trying to make things easier ;)

I also verified I can download manually:

$ gh run download --repo theforeman/foreman-documentation '12830208840' --name foreman-docs-web-master --dir ~/tmp-docs/preview
$ gh run download --repo theforeman/foreman-documentation '12830208840' --name foreman-docs-html-provide_env_var --dir ~/tmp-docs/preview/master
$ ls ~/tmp-docs
preview
$ ls ~/tmp-docs/preview/
404.html  CNAME  css  favicon.ico  img  index.html  js  master  release

Checklists

  • I am okay with my commits getting squashed when you merge this PR.
  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

  • Foreman 3.13/Katello 4.15 (EL9 only)
  • Foreman 3.12/Katello 4.14 (Satellite 6.16)
  • Foreman 3.11/Katello 4.13 (orcharhino 6.11 on EL8 only; orcharhino 7.0 on EL8+EL9)
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8/6.9/6.10)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • We do not accept PRs for Foreman older than 3.7.

Turns out gh already unpacks the artifact so the unzip steps are no
longer needed. This also means we need multiple indivual commands
because they need to be extract to different directories.
Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

Looks sane. Let's try it out.

@maximiliankolb maximiliankolb merged commit 078bc30 into theforeman:master Jan 17, 2025
7 of 8 checks passed
@ekohl ekohl deleted the even-more-preview-work branch January 17, 2025 14:36
mkdir -p preview/${{ fromJSON(env.PR_DATA).target_name }}
unzip -d preview foreman-docs-web-master.zip
unzip -d preview/${{ fromJSON(env.PR_DATA).target_name }} foreman-docs-html-${{ fromJSON(env.PR_DATA).branch_name }}.zip
gh run download '${{ github.event.worfklow_run.id }}' --name 'foreman-docs-web-master' --dir preview
Copy link
Member Author

Choose a reason for hiding this comment

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

Due to a merge conflict, Worf is back. Surprisingly, https://github.com/theforeman/foreman-documentation/actions/runs/12831182509/job/35780996640 passed:

gh run download '' --name 'foreman-docs-web-master' --dir preview

The preview and diff look correct so perhaps the workflow ID is not needed at all and it's smart enough to detect it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Answering my own question from man gh-run-download:

By default, this command downloads the latest artifact created and uploaded through GitHub Actions. Because workflows can delete or overwrite artifacts, must be used to select an artifact from a specific workflow run.

So it worked because it was the last run, but that's not guaranteed to be correct. #3593 should address this.

Overall I think this is now almost done, even though it was bumpy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is a good sign to me: #3592 (comment)

So yes, we should either fix the typo and ignore that it's not really necessary or drop it?

ekohl added a commit to ekohl/foreman-documentation that referenced this pull request Jan 17, 2025
In 078bc30 I reintroduced a typo in the
variable because of a merge conflict, making it end up as an empty
string again. It happened to work because it was the last run (default
behavior) but there's no guarantee this is correct.

Fixes: 078bc30 ("Rely on gh run download to extract the artifacts (theforeman#3591)")
maximiliankolb pushed a commit that referenced this pull request Jan 17, 2025
In 078bc30 I reintroduced a typo in the
variable because of a merge conflict, making it end up as an empty
string again. It happened to work because it was the last run (default
behavior) but there's no guarantee this is correct.

Fixes: 078bc30 ("Rely on gh run download to extract the artifacts (#3591)")
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.

2 participants