[WIP]Implement packages installation APIs for cluster essential#4503
[WIP]Implement packages installation APIs for cluster essential#4503Vandy-P wants to merge 1 commit into
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4503 +/- ##
==========================================
- Coverage 49.77% 48.86% -0.92%
==========================================
Files 453 483 +30
Lines 45424 47544 +2120
==========================================
+ Hits 22612 23230 +618
- Misses 20652 22099 +1447
- Partials 2160 2215 +55 |
codegold79
left a comment
codegold79
left a comment
There was a problem hiding this comment.
Seems pretty straight forward to me. I had a couple questions and minor, optional feedback. None block my approval, so I approve.
| // Also creates missing directories if any | ||
| func SaveFile(filePath string, data []byte) error { | ||
| dirName := filepath.Dir(filePath) | ||
| if _, serr := os.Stat(dirName); serr != nil { |
There was a problem hiding this comment.
I can't tell, but looks like the serr information doesn't get passed along. Do you think that's all right? Maybe people don't need to know what serr is because you pass along merr later? Kind of strange, but I guess it can work.
|
Cluster Generation A/B Results: |
|
Cluster Generation A/B Results: |
Vandy-P
force-pushed
the
cluster-essential
branch
from
7504a57 to
0d8bcf7
Compare
|
Cluster Generation A/B Results: |
Vandy-P
force-pushed
the
cluster-essential
branch
from
0d8bcf7 to
407b872
Compare
Vandy-P
force-pushed
the
cluster-essential
branch
from
407b872 to
25ad233
Compare
|
Cluster Generation A/B Results: |
|
Cluster Generation A/B Results: |
|
Cluster Generation A/B Results: |
|
Cluster Generation A/B Results: |
|
Cluster Generation A/B Results: |
|
Cluster Generation A/B Results: |
|
Question: Why do we have go mod change in other modules ? (Those modules seem unrelated to your change) |
|
@danniel1205 We created kube utility function [util/kube/kube_util.go ] which needs to use client-go v0.25.0 hence all the dependent packages which are using util package got updated with client-go version v0.25.0. |
rajathagasthya
left a comment
rajathagasthya
left a comment
There was a problem hiding this comment.
Suggested some minor changes.
|
Should this be merged with main or feature/runtime-core? |
| } | ||
| var generatedManifestBytes []byte | ||
| for _, item := range packages { | ||
| packageDir := filepath.Join(bundleDir, item.Name()) |
There was a problem hiding this comment.
i'm confused, you are saying this Install function, expects a imgpkg bundle path, but then here you say each package contains a .imgpkg directory, how can a bundle contain multiple .imgpkg directories? how does the structure of imgpkg bundle look like? may be tree structure in the documentation helps
There was a problem hiding this comment.
├── kapp-controller
│ ├── config
│ │ ├── overlays
│ │ │ ├── change-namespace.yaml
│ │ │ ├── update-clusterrole-with-psp.yaml
│ │ │ ├── update-configmap.yaml
│ │ │ ├── update-crds.yaml
│ │ │ ├── update-deployment.yaml
│ │ │ └── update-strategy-overlay.yaml
│ │ ├── upstream
│ │ │ └── kapp-controller.yaml
│ │ ├── values.star
│ │ └── values.yaml
│ └── .imgpkg
│ └── imgpkg.yml
└── secretgen-controller
├── config
│ └── secretgen-controller.yaml
└── .imgpkg
└── imgpkg.yml
There was a problem hiding this comment.
@yharish991 each package is bundle itself
| // This function attempts to install packages from the given imgpkg bundle path | ||
| // and waits until install is complete or the given timeout is reached, whichever | ||
| // occurs first. | ||
| func Install(ctx context.Context, config *rest.Config, clusterEssentialRepo, clusterEssentialVersion string, timeout time.Duration) error { |
There was a problem hiding this comment.
any reason why we want to call this clusterEssentialRepo, it is still a package bundle with some config in it, calling it package bundle makes more sense
There was a problem hiding this comment.
@yharish991 , Its not exactly a package bundle but the repo where cluster essential package reside
Eg:
clusterEssentialRepo := "public.ecr.aws/abcd/cluster-essentials"
clusterEssentialVersion := "v0.0.1"
| if err != nil { | ||
| return err | ||
| } | ||
| err = validatePreInstall(ctx, clientSet, clusterEssentialVersion) |
There was a problem hiding this comment.
should we first do pre-install validation and then download the bundle?
| if !ok { | ||
| return fmt.Errorf("pre validation check failed for package %s, package is not a part of cluster essential", packageName) | ||
| } | ||
| // Get the existing cluster essential version |
There was a problem hiding this comment.
how would you handle the non-cluster-essentials installed kapp/secretgen-controller?
There was a problem hiding this comment.
for non-cluster-essential , prevalidation will fail at line 65
_, ok := deployment.Annotations[packageNameAnnotation]
// If annotation is not present then return error
if !ok {
return fmt.Errorf("pre validation check failed for package %s, package is not a part of cluster essential", packageName)
}
Signed-off-by: Vandana Pathak <pathakv@sjc-ubu-eng-127.vmware.com>
9 participants
What this PR does / why we need it
Implementing APIs for cluster essential installation
Which issue(s) this PR fixes
Fixes #
Describe testing done for PR
Release note
Additional information
Special notes for your reviewer