Skip to content

Github actions#3299

Merged
gotjosh merged 3 commits intoprometheus:mainfrom
jan--f:gha
Sep 21, 2023
Merged

Github actions#3299
gotjosh merged 3 commits intoprometheus:mainfrom
jan--f:gha

Conversation

@jan--f
Copy link
Copy Markdown
Contributor

@jan--f jan--f commented Mar 17, 2023

This is a first draft to move from CircleCI to Github Actions.
Generally I tried to trigger workflows only for relevant changes, e.g. the mixin-lint is only triggered if a file in the mixins change.

Two things are tbd:

  1. This doesn't yet store the frontend asset tarball anywhere. I wasn't exactly sure how this is currently handled in the CircleCI setup, any pointer are appreciated.
  2. This currently uses my fork of maildev at https://github.com/jan--f/maildev/tree/hide-starttls-when-user-password, which simply re-enables STARTTLS, which was disabled in v1.1.1. There is a PR up that looks promising in order for Alertmanager to switch back to upstream maildev. The current PR version doesn't work, since I believe it no longer sets up self-signed certificates. Otoh we could simply create those certs ourselves and pass them via the proposed arguments. This still requires a fix to maildev, see Add option to enabe/disable hideSTARTTLS maildev/maildev#469. So I'll keep my forked image in here for now.

Looking forward to any feedback.

@jan--f
Copy link
Copy Markdown
Contributor Author

jan--f commented Mar 17, 2023

cc @roidelapluie

@simonpasquier
Copy link
Copy Markdown
Member

This doesn't yet store the frontend asset tarball anywhere. I wasn't exactly sure how this is currently handled in the CircleCI setup, any pointer are appreciated.

The test_frontend job will persist the tarball in the workflow's workspace at the same directory (e.g. .tarballs ) where the binaries will be generated. When running promu release ..., everything will be uploaded to the release page (https://github.com/prometheus/alertmanager/releases/tag/v0.26.0).

I believe that we need to use ./.github/promci/actions/save_artifacts and ./.github/promci/actions/restore_artifacts.

@jan--f jan--f force-pushed the gha branch 2 times, most recently from c9e41af to 7720c57 Compare August 29, 2023 16:20
@jan--f
Copy link
Copy Markdown
Contributor Author

jan--f commented Sep 1, 2023

This is currently blocked by actions/runner#2783.

@jan--f
Copy link
Copy Markdown
Contributor Author

jan--f commented Sep 1, 2023

I believe that we need to use ./.github/promci/actions/save_artifacts and ./.github/promci/actions/restore_artifacts.

Thanks! I added ./.github/promci/actions/save_artifacts to the test_frontend job. ./.github/promci/actions/restore_artifacts is then used by the ./.github/promci/actions/publish_[main|release] actions in the publish and release workflows.

@gotjosh
Copy link
Copy Markdown
Member

gotjosh commented Sep 12, 2023

Hey @jan--f can you walk me through the how is this blocked by actions/runner#2783?

@jan--f
Copy link
Copy Markdown
Contributor Author

jan--f commented Sep 12, 2023

@gotjosh sure thing. This intends to run maildev as a service containers.
If you look at the action runs in my clone, the maildev related tests complain about not being able to resolve the service container. according to the docs, this should work exactly as specified though. actions/runner#2783 describes exactly that.

@gotjosh gotjosh marked this pull request as ready for review September 12, 2023 14:27
@gotjosh gotjosh marked this pull request as draft September 12, 2023 14:27
@gotjosh
Copy link
Copy Markdown
Member

gotjosh commented Sep 12, 2023

@jan--f I think we shouldn't block this migration on these tests.

My suggestion is that you can add a new make command that runs all of the excepts except the ones in email_tests.go (we'll run both test suites in parallel for some time before cutting over anyways) and I'll be happy to change the tests to run the container as part of the tests directly (this is something we rely on heavily in mimir).

What do you think?

Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
@jan--f
Copy link
Copy Markdown
Contributor Author

jan--f commented Sep 15, 2023

@gotjosh yeah that sounds good. Happy to take stab at pulling maildev into the test myself if you're not already working on it. We probably still want maildev/maildev#469 though? Or simply keep using the ancient version like in the existing tests?

@jan--f jan--f marked this pull request as ready for review September 15, 2023 11:12
Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
Copy link
Copy Markdown
Member

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

LGTM

on:
pull_request:
paths:
- "doc/alertmanager-mixin/**"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We rarely touch the mixin, but we find out from time to time that something in the ecosystem has been broken and fix it - I believe we should always run it as this is not a very time consuming workflow.

@gotjosh gotjosh merged commit b517645 into prometheus:main Sep 21, 2023
alexweav pushed a commit to grafana/prometheus-alertmanager that referenced this pull request Oct 27, 2023
* Move CI to github actions

Signed-off-by: Jan Fajerski <jfajersk@redhat.com>

* Skip email test in github action

Signed-off-by: Jan Fajerski <jfajersk@redhat.com>

* build before lint

Signed-off-by: Jan Fajerski <jfajersk@redhat.com>

---------

Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
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