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

ApplicationSet CRD size reduction, by removing validation (CRD defn) of nested merge/matrix generator #463

Merged
merged 2 commits into from
Jan 20, 2022
Merged

ApplicationSet CRD size reduction, by removing validation (CRD defn) of nested merge/matrix generator #463

merged 2 commits into from
Jan 20, 2022

Conversation

jgwest
Copy link
Member

@jgwest jgwest commented Jan 18, 2022 •
edited
Loading

See parent issue for detailed description of problem, and proposed solution.

Plus some minor non-functional tweaks:

  • Tweak the name of error constants (add Err prefix) to fix linter warnings in the corresponding files
  • Tweak to the whitespace of boilerplate.go.txt
  • Update organization reference for scm provider tests, to fix GitHub action failure

Fixes #462

@jgwest
fix: ApplicationSet CRD size reduction, by removing validation (CRD d…
2e0c612 
…efn) of nested merge/matrix generator

Signed-off-by: Jonathan West <[email protected]>
@jgwest
fix: ApplicationSet CRD size reduction, by removing validation (CRD d…
d2aac62 
…efn) of nested merge/matrix generator

Signed-off-by: Jonathan West <[email protected]>
@jgwest jgwest marked this pull request as ready for review January 18, 2022 13:31
crenshaw-dev
crenshaw-dev reviewed Jan 19, 2022
View reviewed changes
Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

Overall LGTM. I just have a couple suggestions:

  1. Add a CI check to make sure the CRD doesn't exceed some size threshold. This CI check can be moved to the Argo CD repo when the projects are merged.
  2. Nitpick: Find/replace marshall -> marshal.

wtam2018
wtam2018 reviewed Jan 20, 2022
View reviewed changes
pkg/generators/matrix.go
@@ -68,12 +70,26 @@ func (m *MatrixGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.App
func (m *MatrixGenerator) getParams(appSetBaseGenerator argoprojiov1alpha1.ApplicationSetNestedGenerator, appSet *argoprojiov1alpha1.ApplicationSet) ([]map[string]string, error) {
var matrix *argoprojiov1alpha1.MatrixGenerator
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit picking: Should it be martixGenerator?

@wtam2018 wtam2018 self-requested a review January 20, 2022 20:17
wtam2018
wtam2018 approved these changes Jan 20, 2022
View reviewed changes
Copy link
Collaborator

@wtam2018 wtam2018 left a comment

Choose a reason for hiding this comment

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

LGTM

@wtam2018 wtam2018 merged commit 5787c33 into argoproj:master Jan 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crenshaw-dev crenshaw-dev crenshaw-dev left review comments

@wtam2018 wtam2018 wtam2018 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ApplicationSet CRD size reduction, by removing validation (CRD defn) of nested merge/matrix generator
3 participants
@jgwest @crenshaw-dev @wtam2018