Skip to content

Split up CRD manifests into their own folder#674

Merged
jessesuen merged 1 commit intoargoproj:masterfrom
twz123:separate-crd-manifests
Nov 26, 2018
Merged

Split up CRD manifests into their own folder#674
jessesuen merged 1 commit intoargoproj:masterfrom
twz123:separate-crd-manifests

Conversation

@twz123
Copy link
Member

@twz123 twz123 commented Oct 4, 2018

This way, CRD and app deployments can be separated, e.g. if someone wishes to install Argo CD multiple times.

Also: Make update-manifests.sh fail-fast and more resilient against spaces in paths and user's CDPATH settings.

Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

@twz123

  1. we have a unit test TestGenerateYamlManifestInDir which is dependent on the number of manifests in the base directory. Can you just update the number from 22 to 20 to get the unit test to pass?

  2. There is an e2e test which also depends on the location of the CRD manifests. in fixture.go, this would have to be updated:

func (f *Fixture) setup() error {
	_, err := exec.Command("kubectl", "apply", "-f", "../../manifests/base/application-crd.yaml", "-f", "../../manifests/base/appproject-crd.yaml").Output()

@twz123 twz123 force-pushed the separate-crd-manifests branch from 66f4ecc to 0de75f4 Compare November 23, 2018 13:43
@twz123
Copy link
Member Author

twz123 commented Nov 23, 2018

I fixed the tests. I must have missed the GitHub notification for this one, sorry!

Copy link
Member Author

@twz123 twz123 Nov 23, 2018

Choose a reason for hiding this comment

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

Not sure whether this is a BSD vs GNU thing, but the previous version failed for me on Linux. Hope this works on OSX, too?

Copy link
Member

Choose a reason for hiding this comment

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

Just tried it -- it works on OS X, so you're good.

Copy link
Member

Choose a reason for hiding this comment

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

Oops I spoke too soon, I think we need to follow something like:
https://stackoverflow.com/questions/5694228/sed-in-place-flag-that-works-both-on-mac-bsd-and-linux

to support in-place editing that works on both linux and mac

@codecov-io
Copy link

codecov-io commented Nov 23, 2018

Codecov Report

Merging #674 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #674   +/-   ##
=======================================
  Coverage   28.31%   28.31%           
=======================================
  Files          43       43           
  Lines        6630     6630           
=======================================
  Hits         1877     1877           
  Misses       4471     4471           
  Partials      282      282

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9347f41...eb5fb50. Read the comment docs.

This way, CRD and app deployments can be separated, e.g. if someone wishes to install Argo CD multiple times.

Also: Make update-manifests.sh fail-fast and more resilient against spaces in paths and user's CDPATH settings.
@twz123 twz123 force-pushed the separate-crd-manifests branch from 0de75f4 to eb5fb50 Compare November 23, 2018 13:55
Copy link
Member

Choose a reason for hiding this comment

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

Just tried it -- it works on OS X, so you're good.

@jessesuen jessesuen merged commit b53ad60 into argoproj:master Nov 26, 2018
@twz123 twz123 deleted the separate-crd-manifests branch June 4, 2019 08:43
leoluz pushed a commit to leoluz/argo-cd that referenced this pull request Mar 13, 2025
…om revive (argoproj#674)

Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
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