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

actions: cross build in ci and fix publish #3533

Merged
merged 2 commits into from
Oct 12, 2023

Conversation

jan--f
Copy link
Contributor

@jan--f jan--f commented Sep 22, 2023

This add

  • a job to build on 3 architectures in the CI action
  • Fixes the publish action to cross build all architectures first

TODO: add a release action and discuss and add appropriate trigger filters for release and publish.

@jan--f jan--f force-pushed the gha-fix-publish branch 2 times, most recently from c1c0e4c to e83aa77 Compare September 22, 2023 14:06
@jan--f jan--f changed the title ci action: save artifacts after test run actions: cross build in ci and fix publish Sep 22, 2023
@jan--f jan--f force-pushed the gha-fix-publish branch 3 times, most recently from 08b7b11 to ce0ead4 Compare September 22, 2023 19:31
steps:
- uses: actions/checkout@v3
- uses: prometheus/promci@v0.0.2
- uses: prometheus/promci@3cb0c3871f223bd5ce1226995bd52ffb314798b6 # v0.1.0
Copy link
Member

Choose a reason for hiding this comment

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

Why are we downgrading here? That's wrong with the latest version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually an upgrade from v0.0.2 to v0.1.0 :) The pinning to a sha I copied from Prometheus, seems like good practice and I intend to do that for the other instances too.

Copy link
Member

Choose a reason for hiding this comment

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

🤦 I need to learn to read a bit better - agreed, pinning of versions saves us from unexpected failures.

Comment on lines 35 to 38
docker_hub_login: ${{ secrets.docker_hub_login }}
docker_hub_password: ${{ secrets.docker_hub_password }}
quay_io_organization: ${{ secrets.quay_io_organization }}
quay_io_login: ${{ secrets.quay_io_login }}
quay_io_password: ${{ secrets.quay_io_password }}
Copy link
Member

Choose a reason for hiding this comment

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

None of these secrets are defined. At which point would be ready to test them? How did you approach this in Prometheus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm looking into that, not sure myself at this point. In my clone I defined those secrets but it still fails, there is something I don't understand with multi arch builds.

If those secrets don't exist the publish action is just a no-op. so we can leave this for now and once we understand what's going on, defining the secrets is all that is needed to publish from the action.

Copy link
Member

Choose a reason for hiding this comment

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

Great to know!

Copy link
Member

Choose a reason for hiding this comment

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

they should be defined at the prometheus org level

@gotjosh
Copy link
Member

gotjosh commented Sep 25, 2023

Looking at the current failures, I also see:

The set-output command is deprecated and will be disabled soon. Please upgrade to using Environment Files. For more information see: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

branches:
- main
# branches:
# - main
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just temporary to tigger the action on a PR push. I'll drop all DEBUG commits before this merges.

runs-on: ubuntu-latest
strategy:
matrix:
thread: [ 0, 1, 2 ]
Copy link
Member

Choose a reason for hiding this comment

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

This complexity is not needed for alertmanager

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took this from the circleci build job thinking its a reasonable check before merge. Happy to drop if it isn't needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also #3530 wants to extend this.

- uses: ./.github/promci/actions/publish_main
with:
docker_hub_login: ${{ secrets.docker_hub_login }}
docker_hub_password: ${{ secrets.docker_hub_password }}
quay_io_organization: ${{ secrets.quay_io_organization }}
Copy link
Member

Choose a reason for hiding this comment

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

@roidelapluie is right these all are defined at an org-level except this one - why do we need this one again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again a debug commit to change the org to my quay org.

@gotjosh
Copy link
Member

gotjosh commented Oct 2, 2023

Any chance we could speed this one up @jan--f - I have constant failures on main and would love to avoid those. Thanks!

@jan--f
Copy link
Contributor Author

jan--f commented Oct 4, 2023

@gotjosh sorry, let me drop the debug commits to reduce noise.

@jan--f
Copy link
Contributor Author

jan--f commented Oct 4, 2023

Ok I added the build step before the release action as well.
@roidelapluie @gotjosh Is there anything else needed here?

@gotjosh gotjosh merged commit d7b865d into prometheus:main Oct 12, 2023
10 checks passed
alexweav pushed a commit to grafana/prometheus-alertmanager that referenced this pull request Oct 27, 2023
* actions: cross build in ci and fix publish

Signed-off-by: Jan Fajerski <[email protected]>

* actions: build before release publishing

Signed-off-by: Jan Fajerski <[email protected]>

---------

Signed-off-by: Jan Fajerski <[email protected]>
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.

3 participants