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

chore: update GHA to run workflows for rel-1.5 #373

Open
wants to merge 8 commits into
base: rel-1.5
Choose a base branch
from

Conversation

gatici
Copy link
Contributor

@gatici gatici commented Dec 17, 2024

  • Main workflow is triggered on:
    • Pull requests targeting the rel-1.5 branch.
    • Push operations to the rel-1.5 branch or to any tags that match the pattern v
  • Run staticcheck, linting, license-check and run unit tests
  • Runs E2E tests after passing results from unit-tests, lint, and staticcheck.
  • Builds and pushes a Docker image for testing, executes tests, and cleans up the Docker image afterward.
  • Automatically tags GitHub releases based on changes in the VERSION file if E2E tests passes.
  • Pushes a Docker image for tagged releases to the specified Docker registry (docker.io).
  • Automatically increments the version after a change and creates a pull request to update the VERSION file.

.github/workflows/main.yml Show resolved Hide resolved
.github/workflows/main.yml Show resolved Hide resolved
@gatici gatici requested a review from gab-arrobo December 17, 2024 19:13
@gab-arrobo
Copy link
Contributor

@gatici, I think you need to update the target_branch here: https://github.com/omec-project/amf/blob/main/.github/workflows/push.yml#L113. Also, make sure that the Create Pull Request GHA is getting opened towards the correct target branch (https://github.com/omec-project/amf/blob/main/.github/workflows/push.yml#L164)

@gatici
Copy link
Contributor Author

gatici commented Dec 17, 2024

target_branch here: https://github.com/omec-project/amf/blob/main/.github/workflows/push.yml#L113.

The target_branch is updated here https://github.com/omec-project/amf/blob/main/.github/workflows/push.yml#L113.
https://github.com/omec-project/amf/blob/main/.github/workflows/push.yml#L164 is fine as this is the name of branch which is opened by this job (not the name of target branch).

@gab-arrobo
Copy link
Contributor

target_branch here: https://github.com/omec-project/amf/blob/main/.github/workflows/push.yml#L113.

The target_branch is update here https://github.com/omec-project/amf/blob/main/.github/workflows/push.yml#L113. https://github.com/omec-project/amf/blob/main/.github/workflows/push.yml#L164 is fine as this is the name of branch which is opened by this job (not the name of target branch).

But, given that this automatically opens a PR towards a branch, should we properly update the target branch? I guess the default target branch is main

@gab-arrobo
Copy link
Contributor

target_branch here: https://github.com/omec-project/amf/blob/main/.github/workflows/push.yml#L113.

https://github.com/omec-project/amf/blob/main/.github/workflows/push.yml#L164 is fine as this is the name of branch which is opened by this job (not the name of target branch).

But, given that this automatically opens a PR towards a branch, should we properly update the target branch? I guess the default target branch is main

@gatici, I think we need to provide the base parameter (https://github.com/peter-evans/create-pull-request/blob/main/docs/concepts-guidelines.md#events-which-checkout-a-commit) to indicate the target branch of the PR. otherwise, I think it will be opened towards the default branch, which is main. What do you think?

@gatici
Copy link
Contributor Author

gatici commented Dec 18, 2024

target_branch here: https://github.com/omec-project/amf/blob/main/.github/workflows/push.yml#L113.

https://github.com/omec-project/amf/blob/main/.github/workflows/push.yml#L164 is fine as this is the name of branch which is opened by this job (not the name of target branch).

But, given that this automatically opens a PR towards a branch, should we properly update the target branch? I guess the default target branch is main

@gatici, I think we need to provide the base parameter (https://github.com/peter-evans/create-pull-request/blob/main/docs/concepts-guidelines.md#events-which-checkout-a-commit) to indicate the target branch of the PR. otherwise, I think it will be opened towards the default branch, which is main. What do you think?

Yes, you are right. I added the base parameter. Thank you.

@gab-arrobo
Copy link
Contributor

Hi @thakurajayL @gruyaume,
@gatici opened PR #369 that is pointing towards a release branch (i.e., branch: rel-1.5) and we noticed that the GHAs are not handling cases when PRs are directed towards release branches, and I think this is not something we discussed when "re-designing" the CI/CD pipeline. We talked about creating the release branches, but we did not provide specifics on how to handle such PRs. So, to address this limitation, @gatici opened this PR (#373).

Currently, we have 5 GHAs in push.yml (push-images, tag-github, release-image, update-version, branch-release), where the push-images GHA takes care of pushing per-PR images (tags: main-commitID and main-latest) towards the Aether registry (https://registry.aetherproject.org/harbor/projects/9/repositories/5gc-amf/artifacts-tab). For the case of release branches, do you think we need to push these per-PR images? Gulsum is properly handling this such that the image tag would be rel-1.5-commitID and rel-1.5-latest. However, I do not think we need to push these images and instead we should do something like follows to skip the creation of these non-release images. What do you think?

diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml
index 031ff5e..1f966f8 100644
--- a/.github/workflows/push.yml
+++ b/.github/workflows/push.yml
@@ -12,7 +12,7 @@ on:
 jobs:
   push-images:
     runs-on: ubuntu-latest
-    if: github.repository_owner == 'omec-project'
+    if: (github.repository_owner == 'omec-project') && (github.ref_name == 'main')
     env:
       REGISTRY: registry.aetherproject.org
       DOCKER_REGISTRY: registry.aetherproject.org/

Also, one aspect that @thakurajayL pointed out in a similar PR (omec-project/nrf#165 (comment)) is that VERSION file has the value of the "future" version. That is, for release branch rel-1.5, VERSION's content is 1.6.0, and VERSION's content for rel-1.4 branch is 1.5.0, which will be confusing for users trying to create PRs towards the release branches. Any thoughts on how to proceed with this?

Also adding @ghislainbourgeois for his thoughts/feedback on this.

@ghislainbourgeois
Copy link
Contributor

Hi @thakurajayL @gruyaume, @gatici opened PR #369 that is pointing towards a release branch (i.e., branch: rel-1.5) and we noticed that the GHAs are not handling cases when PRs are directed towards release branches, and I think this is not something we discussed when "re-designing" the CI/CD pipeline. We talked about creating the release branches, but we did not provide specifics on how to handle such PRs. So, to address this limitation, @gatici opened this PR (#373).

Currently, we have 5 GHAs in push.yml (push-images, tag-github, release-image, update-version, branch-release), where the push-images GHA takes care of pushing per-PR images (tags: main-commitID and main-latest) towards the Aether registry (https://registry.aetherproject.org/harbor/projects/9/repositories/5gc-amf/artifacts-tab). For the case of release branches, do you think we need to push these per-PR images? Gulsum is properly handling this such that the image tag would be rel-1.5-commitID and rel-1.5-latest. However, I do not think we need to push these images and instead we should do something like follows to skip the creation of these non-release images. What do you think?

diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml
index 031ff5e..1f966f8 100644
--- a/.github/workflows/push.yml
+++ b/.github/workflows/push.yml
@@ -12,7 +12,7 @@ on:
 jobs:
   push-images:
     runs-on: ubuntu-latest
-    if: github.repository_owner == 'omec-project'
+    if: (github.repository_owner == 'omec-project') && (github.ref_name == 'main')
     env:
       REGISTRY: registry.aetherproject.org
       DOCKER_REGISTRY: registry.aetherproject.org/

Also, one aspect that @thakurajayL pointed out in a similar PR (omec-project/nrf#165 (comment)) is that VERSION file has the value of the "future" version. That is, for release branch rel-1.5, VERSION's content is 1.6.0, and VERSION's content for rel-1.4 branch is 1.5.0, which will be confusing for users trying to create PRs towards the release branches. Any thoughts on how to proceed with this?

Also adding @ghislainbourgeois for his thoughts/feedback on this.

I think the CI as it is might be a bit problematic because it is lacking tests on the generated image. For now I think it would be fine to not push the PR image to the aether registry, but in the future it would be great to follow a process that looks more or less like this:

build-image -> test-image -> release-image

where test-image and release-image do not build anything but use the artifact from build-image. For that, we could use the aether registry as a temporary place to store the image while it is going through CI, but we could also use the github registry. In any case, automating some cleanup of old images would be useful.

Regarding the VERSION file, I think it should reflect the current version. It would mean changes to the CI going forward, but we could use it as a trigger to cut a release, meaning that when we are ready to cut a new release, we create a PR that bumps the VERSION, and merging that could automatically do things like creating a new branch, tagging the release and publishing the images.

I would be happy to help on this, but it is probably a larger effort than this PR. I can write up a document to discuss at a TST meeting in the new year to agree on an approach maybe?

@gab-arrobo
Copy link
Contributor

@ghislainbourgeois thanks for your feedback. Please see below my comments:

I think the CI as it is might be a bit problematic because it is lacking tests on the generated image. For now I think it would be fine to not push the PR image to the aether registry, but in the future it would be great to follow a process that looks more or less like this:

build-image -> test-image -> release-image

where test-image and release-image do not build anything but use the artifact from build-image. For that, we could use the aether registry as a temporary place to store the image while it is going through CI, but we could also use the github registry. In any case, automating some cleanup of old images would be useful.

Yes, I was working on something like this (testing the image) but I have not had a chance to achieve much progress due to many other pending/higher priority tasks (Here you can see what I have committed so far and I have some other change in my local system: https://github.com/gab-arrobo/amf/tree/gha-e2e)

Regarding the VERSION file, I think it should reflect the current version. It would mean changes to the CI going forward, but we could use it as a trigger to cut a release, meaning that when we are ready to cut a new release, we create a PR that bumps the VERSION, and merging that could automatically do things like creating a new branch, tagging the release and publishing the images.

This is what it is kind of happening in the current CI/CD pipeline. Feel free to take a closer look at the push.yml file to see this.

I would be happy to help on this, but it is probably a larger effort than this PR. I can write up a document to discuss at a TST meeting in the new year to agree on an approach maybe?

FYI, Guillaume shared a document about this that we used for the "re-design" of the pipeline. Will the link with you through Slack

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