Skip to content
This repository has been archived by the owner on Jul 15, 2024. It is now read-only.

feat: support merge generator #404

Merged
merged 58 commits into from
Dec 6, 2021

Conversation

crenshaw-dev
Copy link
Member

fixes #344

@CLAassistant
Copy link

CLAassistant commented Nov 2, 2021

CLA assistant check
All committers have signed the CLA.

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
… version

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
main.go Outdated Show resolved Hide resolved
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
pkg/generators/git.go Outdated Show resolved Hide resolved
pkg/generators/pull_request.go Outdated Show resolved Hide resolved
pkg/generators/scm_provider.go Outdated Show resolved Hide resolved
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
@crenshaw-dev crenshaw-dev changed the title feat: support union generator feat: support merge generator Nov 12, 2021
Copy link
Member

@jgwest jgwest left a comment

Choose a reason for hiding this comment

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

Looks great @crenshaw-dev! Really nice, detailed work 🎉.

Just a couple of requests:

  • It would be good to get an E2E test for the merge generator, as we have for the other generators. It should be quick to put it together if you base it on the matrix_e2e_test.go
  • Plus some quick tweaks mentioned inline in the review.

(As you said below, the need for nesting makes things a bit convoluted, but that's a necessary evil due to CRD's lack of support for recursion, and our desire to continue to support CRD validation and type definitions.)

pkg/utils/map_test.go Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/Generators-Merge.md Outdated Show resolved Hide resolved
pkg/generators/merge.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
@crenshaw-dev
Copy link
Member Author

I'm confused by the build failure. The field which is missing from the committed manifests was recently added (argoproj/argo-cd#7249). But as far as I can tell, it wasn't added to the argo-cd version in go.sum (2.1.0).

Even weirder (to me) is that the logs don't mention downloading argo-cd at all. Guess I need to learn more about go modules.

@jgwest
Copy link
Member

jgwest commented Nov 23, 2021

@crenshaw-dev Don't worry about it, I think what you have is good, I'll take a look and fix it as needed (probably related to my go.mod bump on master late last week)

@crenshaw-dev
Copy link
Member Author

@jgwest cool, thanks. I still need to add the e2e test, which I should be able to do this afternoon.

@jgwest
Copy link
Member

jgwest commented Nov 30, 2021

@crenshaw-dev - any luck with this, and/or any issues that you are hitting that I can help with? I'm working on the ApplicationSet v0.3.0 release, and it would be good to get this in if you think that's possible, but no pressure 😄.

@crenshaw-dev
Copy link
Member Author

@jgwest I've been unable to get my e2e test env. working. I'll start a Slack thread. I'm sure I'm missing something simple. :-)

@jgwest
Copy link
Member

jgwest commented Nov 30, 2021

@crenshaw-dev One other issue I've noticed is that the ApplicationSet controller install YAML can no longer be applied; I see now why you changed the install script from kubectl create to kubectl apply.., with the error being "The CustomResourceDefinition "applicationsets.argoproj.io" is invalid: metadata.annotations: Too long: must have at most 262144 bytes"

@crenshaw-dev
Copy link
Member Author

@jgwest yep... I'll double-check that there's no documentation which requires apply instead of create.

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
@crenshaw-dev
Copy link
Member Author

@jgwest added e2e tests. You'll probably also want to glance at the docs commit.

@crenshaw-dev
Copy link
Member Author

Looks like the doc lint error is a cert issue on kustomize.io. My guess is they'll fix the cert soon. Probably safe to ignore the error.

@jgwest
Copy link
Member

jgwest commented Dec 2, 2021

@crenshaw-dev -

Switching from kubectl apply to kubectl create has a few major issues:

  • It makes upgrade more difficult: we would need to ask the user to kubectl delete && kubectl create their application set install, vs the current model which is an in-place upgrade with apply. The in-place apply is how both applicationset controller and Argo CD work now.
    • I'm not even sure if it's possible to delete a CRD if there are CRs that are still using it? IIRC I've seen mixed results here in the past. It means, to upgrade, you might need to delete all the ApplicationSet CRs as well.
  • As you are aware, we are working on merging ApplicationSet controller into Argo CD. So this change would also affect Argo CD, because the ApplicationSet CR would now be a part of the Argo CD install yaml. Thus we would require Argo CD users to kubectl delete && kubectl create upgrade Argo CD using the create/delete approach as well.

@alexmt have y'all hit this with Argo CD CR, and/or any suggestions? This would unfortunately affect Argo CD as wel,l if we were to combine argo cd/appset install YAMLs.

@crenshaw-dev
Copy link
Member Author

I'm a bit ignorant of the subtleties here... Couldn't a user perform a non-disruptive upgrade using kubectl replace?

crenshaw-dev and others added 5 commits December 2, 2021 15:46
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Copy link
Member

@jgwest jgwest left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again @crenshaw-dev, great work.

(And thanks @leoluz for reviewing!)

@jgwest jgwest merged commit 398f460 into argoproj:master Dec 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support 'union' generator
4 participants