Implement packages cluster spec #5031
Conversation
eks-distro-bot
added
the
size/L
TerryHowe
force-pushed
the
implement-packages-cluster-spec
branch
from
4b8d731
to
3bc942d
Compare
Codecov Report
@@ Coverage Diff @@
## main #5031 +/- ##
==========================================
+ Coverage 72.07% 72.13% +0.05%
==========================================
Files 421 421
Lines 34396 34466 +70
==========================================
+ Hits 24792 24862 +70
Misses 8085 8085
Partials 1519 1519
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -72,7 +72,7 @@ func installPackageController(ctx context.Context) error { | |||
| } | |||
|
|
|||
| curatedpackages.PrintLicense() | |||
| err = ctrlClient.EnableCuratedPackages(ctx) | |||
| err = ctrlClient.EnableCuratedPackages(ctx, &clusterSpec.Cluster.Spec) | |||
There was a problem hiding this comment.
I think injecting this into ctrlClient will make this so much easier.
|
|
||
| if clusterSpec.Packages.Controller != nil { | ||
| result += "controller:\n" | ||
| result += formatYamlLine(" ", "digest", clusterSpec.Packages.Controller.Digest) |
There was a problem hiding this comment.
Is the goal to return a yaml formatted controller config? Maybe yaml marshaller will come in handy here?
There was a problem hiding this comment.
I tried taht, but this approach was much easier to understand and test.
There was a problem hiding this comment.
I have used the yaml marshall's functionality and it was easy to understand and also test. We can sync up offline.
|
/test eks-anywhere-e2e-presubmit |
1 similar comment
|
/test eks-anywhere-e2e-presubmit |
| } | ||
|
|
||
| // GetPackageControllerConfiguration returns the default kubernetes version for a Cluster. | ||
| func GetPackageControllerConfiguration(clusterSpec *v1alpha1.ClusterSpec) (result string, err error) { |
There was a problem hiding this comment.
I would recommend moving this method to pkgControllerClient since functionally this method is dealing with a packagecontroller.
| @@ -149,7 +161,7 @@ func newPackageControllerTests(t *testing.T) []*packageControllerTest { | |||
| kubectl: k, | |||
| chartInstaller: ci, | |||
| command: curatedpackages.NewPackageControllerClient( | |||
| ci, k, clusterName, kubeConfig, chartDev, | |||
| ci, k, clusterName, nil, kubeConfig, chartDev, | |||
There was a problem hiding this comment.
From usage, looks like clusterSpec is not used by all. Should we make it an option in PCC?
There was a problem hiding this comment.
it is only used in the generate yaml method. I don't want to create one for tests that don't use it.
There was a problem hiding this comment.
What I am referring to is in the NewPackageControllerClient should we make the clusterSpec a PackageControllerClientOpt so that you don't need to pass the nil everywhere.
| } | ||
|
|
||
| if clusterSpec.Packages.Controller != nil { | ||
| result += "controller:\n" |
There was a problem hiding this comment.
I am still not comfortable with custom parsing instead of using the classic yaml marshaller but if you think this is the better way, I guess it is fine then.
TerryHowe
force-pushed
the
implement-packages-cluster-spec
branch
from
1c6104b
to
cb64580
Compare
TerryHowe
force-pushed
the
implement-packages-cluster-spec
branch
from
cb64580
to
0cc7185
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-cool-train The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.