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

Migrate from CircleCI to GitHub Actions #565

Closed
wants to merge 4 commits into from

Conversation

jsoref
Copy link
Member

@jsoref jsoref commented Jan 29, 2021

Circle CI is a lot harder for contributors to use than just using native GitHub actions. It's also unfortunately somewhat flakey and quirky – if a contributor has a Circle CI account and the host had a paid account, then a PR might get a failure because of a run for the submitter's instance hitting limits. If one doesn't have a Circle CI account, then they can't test their work until they make a PR. Catch 22.

Checklist:

  • I have update the chart version in Chart.yaml following Semantic Versioning.
  • Any new values are backwards compatible and/or have sensible default.
  • I have followed the testing instructions in the contributing guide.
  • I have signed the CLA and the build is green. -- The fact that the build is green is mostly a lie
    -- This .circleci/config.yml is intentionally left blank.
  • I will test my changes again once merged to master and published.

Changes are automatically published when merged to master. They are not published on branches.

Note: while this is true, with this PR, they are generated and an index file will be added to the local github repository -- the only thing that won't be performed is the actual push itself.

It is possible to switch from using container images to using GitHub actions for the other bits. But this is a minimal viable product to migrate to GitHub Actions.

The linter screamed very loudly when I didn't bump the api version of the CustomResourceDefinitions

I'm changing to quay.io as ArgoCD is doing this in general, and relying on docker.io in CI is asking for flaky builds (I've hit some making drive-by PRs to other projects).

Copy link
Contributor

@yann-soubeyrand yann-soubeyrand left a comment

Choose a reason for hiding this comment

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

Firstly, thanks for your work!

I'm not familiar with the CI of this repo, however I'm wondering why you keep the .circleci folder, why don't you move the ct configuration file out of it and simply remove the .circleci folder?

Changing the image source to quay.io is a good thing. It could have been the subject of another PR, but since you made the change in a dedicated commit, I think it's OK (I let more experienced reviewers give their opinion on it ;-)).

Regarding the CRDs modification, I would say that we shouldn't do this if it's not done upstream. CRDs in these charts should remain synchronised with CRDs provided upstream.

@jsoref
Copy link
Member Author

jsoref commented Feb 1, 2021

The lack of the rename was to have a minimal set of changes.
Totally happy to rename it -- not sure what to call it / where to put it.
Note that I can't actually delete the directory in this PR as circleci fails if the intentionally empty file isn't present. So, I couldn't actually delete it as is. I'd half argue that it could be moved in the followup that removes circleci once the integration is removed from circleci.

This partially a proof of concept / discussion point.

The CRD change was because the integration didn't seem to like me without it. Totally fine w/ sending it upstream (pointers welcome, I'm quite scattered right now).

@yann-soubeyrand
Copy link
Contributor

The CRD change was because the integration didn't seem to like me without it. Totally fine w/ sending it upstream (pointers welcome, I'm quite scattered right now).

I guess the files to modify are here (I don't think they are otherwise generated):

Copy link
Contributor

@seanson seanson left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, this is looking pretty good! Could we pop the reason that we'd like to switch from CircleCI to GitHub Actions into the PR description so it's clear why we'd like to make this change?

Additionally with CRD changes it would be good to get someone to confirm they're not impacted by the version changes with a test install. I believe Helm should take care of it with a CreateMergePatch but some user validation would be good.

@chgl
Copy link
Contributor

chgl commented Feb 25, 2021

Unfortunately bumping the apiextensions version to v1 isn't enough as there were some backwards-incompatible changes from beta to v1. See:

$kubectl apply --dry-run=client -f charts/argo/crds/cluster-workflow-template-crd.yaml
error: error validating "charts/argo/crds/cluster-workflow-template-crd.yaml": error validating data: ValidationError(CustomResourceDefinition.spec): unknown field "version" in io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.CustomResourceDefinitionSpec; if you choose to ignore these errors, turn validation off with --validate=false

See for example my PR over at: argoproj/argo-cd#5516. I think the best approach is to first update the tooling used to generate these CRDs in the source repos and then adopt them in the associated helm charts. Until then disabling the lint/deprecation check temporarily would be helpful.

As part of the CI, we could also use ct install for a basic smoke test in KinD (or k3s, or k3d). I've had some success with it over at https://github.com/chgl/charts/blob/master/.github/workflows/ci.yaml#L36

@jsoref
Copy link
Member Author

jsoref commented Feb 25, 2021

If we could disable the lint checking so that this PR can be merged without waiting for the CRDs to be fixed, that'd be great... As is this repository is basically a trap (It's a trap!) in that PRs to it will fall through no fault of contributors. Even for simple things like spelling fixes...

Fixing CRDs is a lot to ask of drive-by contributors.

@github-actions
Copy link

Stale pull request message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants