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

Decommission circleci #5730

Merged
merged 7 commits into from
Nov 30, 2022
Merged

Conversation

beni0888
Copy link
Collaborator

Description of the change

This commit decommissions the CircleCI pipeline and removes the temporary logic we added for development purposes from the GHA one.

Benefits

We get rid of the CircleCI pipeline and stick to the GHA one.

Possible drawbacks

None.

Applicable issues

Jesús Benito Calzada added 3 commits November 25, 2022 17:51
Signed-off-by: Jesús Benito Calzada <bjesus@vmware.com>
Signed-off-by: Jesús Benito Calzada <bjesus@vmware.com>
Signed-off-by: Jesús Benito Calzada <bjesus@vmware.com>
@beni0888 beni0888 marked this pull request as ready for review November 25, 2022 16:57
@netlify
Copy link

netlify bot commented Nov 25, 2022

Deploy Preview for kubeapps-dev canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 2d83a23
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/638722c0bd30ab00088e9d15

@beni0888 beni0888 self-assigned this Nov 25, 2022
@beni0888 beni0888 added component/ci Issue related to kubeapps ci system github_actions Pull requests that update GitHub Actions code labels Nov 25, 2022
@beni0888 beni0888 added this to the Migrate CI to GitHub Actions milestone Nov 25, 2022
Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

Whoa!! Finally, this moment arrived! Thanks!

IMG_PREFIX: "kubeapps/"
# We use IMG_PREFIX_FOR_FORKS for development purposes, it's used when the workflow is run from a fork of the kubeapps repo
IMG_PREFIX_FOR_FORKS: "beni0888/"
# IMG_PLATFORMS: "linux/amd64, linux/arm64"
IMG_PREFIX_FOR_FORKS: "johndoe/"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use the kubeapps-bot account instead, just in case:

Suggested change
IMG_PREFIX_FOR_FORKS: "johndoe/"
IMG_PREFIX_FOR_FORKS: "kubeapps-bot/"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I chose johndoe on purpose to make clear that it is something external to the Kubeapps organization. IMHO using kubeapps-bot could lead to confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

your-dockerhub-username?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just asking: what if someone creates a your-dockerhub-username or a johndoe username in GitHub? Can they perform some undesired actions against our CI ? Or is it 100% safe ?

If there is a minimum risk of an undesired action, I'd rather go with something we have control over, like the bot user. If not, anything you suggest seems ok to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't understand exactly what you meant at first @antgamdia, as I thought the prefix is just used to push images when running in a forked repo, in which case it would fail anyway, but I think what you're suggesting is that maybe there is some way that us configuring a valid name that we don't own could lead to a privilege of that account? I don't see how, but I guess that's the point. Anyway, if so, why not give it an invalid name, like "" (assuming the <> are invalid).

Anyway, I think this point we're discussing here is probably not that important compared to the actual change, so still +1 from me, whatever you decide @beni0888 .

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@antgamdia I think you're not really understanding the meaning of that variable 🙂 Its only purpose is to allow you to push the images to a different workspace in DockerHub other than kubeapps, on which you (the user running the pipeline on its own fork) won't have permissions to push. It was added mainly for developing purposes, as I didn't want to push images from my fork to DockerHub at the early stages of the migration. But I cannot see any chance of performing an undesired action thanks to the value of this variable. BTW, I think that @absoludity suggestion of using your-dockerhub-username instead of johndoe is a good idea 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh, got it, got it. Thanks for the clarification! Sorry for the confusion :P
+1 to your-dockerhub-username or even your-container-registry-username

@absoludity
Copy link
Contributor

Hi @beni0888 . The checks here just have one error, which I assume is related to removing the circleCI pipeline? If so, is there anything stopping this from landing? I'd like to start the next release soon, but am keen for this to land before I do so. Thanks either way!

@beni0888
Copy link
Collaborator Author

Hi @beni0888 . The checks here just have one error, which I assume is related to removing the circleCI pipeline? If so, is there anything stopping this from landing? I'd like to start the next release soon, but am keen for this to land before I do so. Thanks either way!

No, there's nothing preventing us from landing this. I will apply your suggestion of using you-dockerhub-username in IMG_PREFIX_FOR_FORKS and tune the branch protection rules to remove those for CircleCI and add the GHA ones, and then will merge this.

Jesús Miguel Benito Calzada added 2 commits November 30, 2022 10:26
Signed-off-by: Jesús Benito Calzada <bjesus@vmware.com>
@beni0888 beni0888 merged commit b915873 into vmware-tanzu:main Nov 30, 2022
@beni0888 beni0888 deleted the decommission-circleci branch November 30, 2022 09:54
absoludity added a commit that referenced this pull request Dec 6, 2022
Also updates Hugo version (based on instructions in the
release-process.md.

Signed-off-by: Michael Nelson <minelson@vmware.com>
absoludity added a commit that referenced this pull request Dec 6, 2022
This PR aims to fix the failing full integration pipeline (which we need
to pass for release).

Also updates Hugo version (based on instructions in the
release-process.md.

Signed-off-by: Michael Nelson <minelson@vmware.com>

<!--
Before you open the request please review the following guidelines and
tips to help it be more easily integrated:

 - Describe the scope of your change - i.e. what the change does.
 - Describe any known limitations with your change.
- Please run any tests or examples that can exercise your modified code.

 Thank you for contributing!
 -->

### Description of the change

<!-- Describe the scope of your change - i.e. what the change does. -->
Recent runs (from the past 6 days) of the [full integration
pipeline](https://github.com/vmware-tanzu/kubeapps/actions/workflows/kubeapps-full-integration.yaml)
have been failing. Looking for the differences, I could see that:

- `chart-museum.sh` was failing to upload our custom apache chart to the
chartmuseum instance, because...
- `LOAD_BALANCER_IP` was set to `172.18.0.2`, which is the `DEX_IP`,
whereas it should be set to the load balancer IP address (which has been
created correctly), as it is in the last passing run. It's not doing
this because...
- The code that sets the `LOAD_BALANCER_IP` address does so
conditionally based on the `GKE_BRANCH` environment variable, which is
*blank*... so as far as that script can tell, it's not a GKE run. The
reason that the `GKE_BRANCH` env var is blank is because...
- In
[#5730](https://github.com/vmware-tanzu/kubeapps/pull/5730/files#diff-164a87eb83a2101307c123bd6651d8f9355b51a995971d260038a1be44d6f6dbL19)
the `GKE_BRANCH` env var was removed and replaced with `GKE_VERSION`
everywhere *except* `e2e-test.sh` it seems. I assume this was just an
oversight on our part.

So this PR just finishes that change by renaming the var in
`e2e-test.sh`.

### Benefits

<!-- What benefits will be realized by the code change? -->

Hopefully we can now pass the full integration pipeline.

### Possible drawbacks

<!-- Describe any known limitations with your change -->

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- fixes #

### Additional information

<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->

Signed-off-by: Michael Nelson <minelson@vmware.com>
antgamdia added a commit that referenced this pull request Feb 2, 2024
### Description of the change

Following up #5730, this PR
finally removes the old CircleCI config, as it hasn't been useful
recently.

### Benefits

No outdated files.

### Possible drawbacks

N/A

### Applicable issues

N/A

### Additional information

N/A

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-not-required component/ci Issue related to kubeapps ci system github_actions Pull requests that update GitHub Actions code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants