Resolved the issue where patch file can't point to a resource based on a name #385
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jayanth-tjvrr
requested review from
cruizen,
bcle,
Rohitrajak1807 and
ShaunakJoshi1407
Codecov Report
@@ Coverage Diff @@
## main #385 +/- ##
==========================================
+ Coverage 28.71% 29.33% +0.62%
==========================================
Files 25 25
Lines 1654 1619 -35
==========================================
Hits 475 475
+ Misses 1132 1097 -35
Partials 47 47
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
bcle
requested changes
Dec 20, 2022
cruizen
reviewed
Dec 20, 2022
docs/gen2_Tutorial.md
Outdated
| @@ -375,24 +375,35 @@ arlon cluster create --cluster-name <clusterName> --repo-alias prod --repo-path | |||
|
|
|||
| We call the concept of constructing various clusters with patches from the same base manifest as cluster overrides. | |||
| The cluster overrides feature is built on top of the existing base cluster design. So, A user can create a cluster from the base manifest using the same command as in the above step(gen2 cluster creation). | |||
| Now, to create a cluster with overrides in the base manifest, a user should have the corresponding patch files in a dedicated folder in local which doesn't contain any other files except patch files. Example of a patch file where we want to override replicas count to 2 is: | |||
| Now, to create a cluster with overrides in the base manifest, a user should have the corresponding patch files in a single yaml file in local. Example of a patch file where we want to override replicas count to 2 is and change the sshkeyname: | |||
There was a problem hiding this comment.
Suggested change
| Now, to create a cluster with overrides in the base manifest, a user should have the corresponding patch files in a single yaml file in local. Example of a patch file where we want to override replicas count to 2 is and change the sshkeyname: | |
| Now, to create a cluster with overrides in the base manifest, a user should have the corresponding patch files in a single yaml file in local. Here is an example of a patch file where we want to override replicas count to 2 and change the sshkeyname: |
cruizen
reviewed
Dec 20, 2022
docs/gen2_Tutorial.md
Outdated
| Refer to this [document](https://blog.scottlowe.org/2019/11/12/using-kustomize-with-cluster-api-manifests/) to know more about patch files | ||
|
|
||
| Command to create a gen2 workload cluster form the base cluster manifest with overrides to the manifest is: | ||
|
|
||
| ```shell | ||
| arlon cluster create <cluster-name> --repo-url <repo url where base manifest is present> --repo-path <repo path to the base manifest> --override <path to the patch files folder> --patch-repo-url <repo url where patch files should be stored> --patch-repo-path <repo path to store the patch files> | ||
| arlon cluster create <cluster-name> --repo-url <repo url where base manifest is present> --repo-path <repo path to the base manifest> --overrides-path <path to the patch files folder> --patch-repo-url <repo url where patch files should be stored> --patch-repo-path <repo path to store the patch files> |
There was a problem hiding this comment.
Suggested change
| arlon cluster create <cluster-name> --repo-url <repo url where base manifest is present> --repo-path <repo path to the base manifest> --overrides-path <path to the patch files folder> --patch-repo-url <repo url where patch files should be stored> --patch-repo-path <repo path to store the patch files> | |
| arlon cluster create <cluster-name> --repo-url <repo url where base manifest is present> --repo-path <repo path to the base manifest> --overrides-path <path to the patch file> --patch-repo-url <repo url where patch file should be stored> --patch-repo-path <repo path to store the patch file> |
cruizen
reviewed
Dec 20, 2022
pkg/gitutils/manifests.go
Outdated
| @@ -168,9 +104,25 @@ func CopyPatchManifests(wt *gogit.Worktree, filePath string, clusterPath string, | |||
| return fmt.Errorf("failed to create kustomization.yaml: %s", err) | |||
| } | |||
| err = tmpl.Execute(file, yamlData) | |||
| if err != nil { | |||
| return fmt.Errorf("failed to execute kustomization.yaml manifest: %s", err) | |||
There was a problem hiding this comment.
Suggested change
| return fmt.Errorf("failed to execute kustomization.yaml manifest: %s", err) | |
| return fmt.Errorf("failed to execute kustomization.yaml manifest") |
jayanth-tjvrr
force-pushed
the
private/main/jayanth-reddy/ResolvingOverrideIssue
branch
from
74ac6ee to
e5c8cc5
Compare
bcle
requested changes
Dec 20, 2022
pkg/gitutils/manifests.go
Outdated
| @@ -168,9 +104,25 @@ func CopyPatchManifests(wt *gogit.Worktree, filePath string, clusterPath string, | |||
| return fmt.Errorf("failed to create kustomization.yaml: %s", err) | |||
| } | |||
| err = tmpl.Execute(file, yamlData) | |||
| if err != nil { | |||
| return fmt.Errorf("failed to execute kustomization.yaml manifest") | |||
| } | |||
| _ = file.Close() | |||
There was a problem hiding this comment.
Move this to above 106 with a defer
pkg/gitutils/manifests.go
Outdated
| return fmt.Errorf("failed to create destination file %s: %s", dstPath, err) | ||
| } | ||
| _, err = io.Copy(dst, src) | ||
| _ = src.Close() |
There was a problem hiding this comment.
Not needed if deferred above.
pkg/gitutils/manifests.go
Outdated
| dstPath := path.Join(clusterPath, fileName) | ||
| dst, err := wt.Filesystem.Create(dstPath) | ||
| if err != nil { | ||
| _ = src.Close() |
There was a problem hiding this comment.
Not needed if deferred above.
pkg/gitutils/manifests.go
Outdated
| } | ||
| _, err = io.Copy(dst, src) | ||
| _ = src.Close() | ||
| _ = dst.Close() |
There was a problem hiding this comment.
Move to before 120 with defer
bcle
approved these changes
Dec 20, 2022
| @@ -167,10 +104,24 @@ func CopyPatchManifests(wt *gogit.Worktree, filePath string, clusterPath string, | |||
| if err != nil { | |||
| return fmt.Errorf("failed to create kustomization.yaml: %s", err) | |||
| } | |||
| defer file.Close() | |||
There was a problem hiding this comment.
General comment:
Can you change the defers to: defer _ = xxx.Close() or else my IDE will complain about the return value not being used. Not sure what IDE you use.
Rohitrajak1807
deleted the
private/main/jayanth-reddy/ResolvingOverrideIssue
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the present overrides design, a user can't point to a single resource in resources file using the name of resource. The patch gets applied to all the resources irrespective of the name field in patch.yaml. This PR fixes this issue and now, user can apply a patch to a particular resource using the name field in patch.yaml. Deployed a cluster from the branch and it turned out to be healthy and synced with the patch changes.
