Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logic to build cluster.Spec from cluster #2402

Merged
merged 1 commit into from
Jun 14, 2022

Conversation

g-gaston
Copy link
Member

@g-gaston g-gaston commented Jun 13, 2022

Issue #, if available: part of https://github.com/aws/eks-anywhere-internal/issues/1184 and #1109

Description of changes:

Builds (fully) a cluster.Spec using a kubernetes client:

  • Builds the cluster.Config with the default builder
  • Retrieves Bundles and the eksd release from the cluster
  • Some data massaging to complete the Spec

Please, look at comments in PR for more localized explanations.

Testing (if applicable):

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@g-gaston
Copy link
Member Author

/hold

@eks-distro-bot eks-distro-bot added do-not-merge/hold size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 13, 2022
@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #2402 (68dd3d9) into main (a79a36a) will increase coverage by 0.26%.
The diff coverage is 85.24%.

❗ Current head 68dd3d9 differs from pull request most recent head 42e6a36. Consider uploading reports for the commit 42e6a36 to get more accurate results

@@            Coverage Diff             @@
##             main    aws/eks-anywhere#2402      +/-   ##
==========================================
+ Coverage   56.25%   56.52%   +0.26%     
==========================================
  Files         304      305       +1     
  Lines       24689    24773      +84     
==========================================
+ Hits        13889    14002     +113     
+ Misses       9507     9475      -32     
- Partials     1293     1296       +3     
Impacted Files Coverage Δ
pkg/cluster/spec.go 60.88% <76.00%> (+5.52%) ⬆️
pkg/cluster/fetch.go 45.00% <91.66%> (+23.26%) ⬆️
pkg/providers/snow/fetch.go 100.00% <0.00%> (ø)
pkg/clusterapi/fetch.go 100.00% <0.00%> (ø)
pkg/clusterapi/apibuilder.go 93.20% <0.00%> (+0.10%) ⬆️
pkg/providers/snow/apibuilder.go 84.87% <0.00%> (+0.20%) ⬆️
pkg/clusterapi/name.go 89.28% <0.00%> (+1.78%) ⬆️
pkg/providers/snow/objects.go 81.89% <0.00%> (+2.72%) ⬆️
... and 2 more

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 a79a36a...42e6a36. Read the comment docs.

@g-gaston g-gaston force-pushed the spec-from-cluster branch 4 times, most recently from 74cc71c to 02ce1cd Compare June 14, 2022 00:32
@eks-distro-bot eks-distro-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 14, 2022
@g-gaston g-gaston force-pushed the spec-from-cluster branch 4 times, most recently from 1c2d9f5 to 277e13c Compare June 14, 2022 02:14
@eks-distro-bot eks-distro-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 14, 2022
@g-gaston g-gaston force-pushed the spec-from-cluster branch 2 times, most recently from 709b6d1 to 0f03aef Compare June 14, 2022 02:32
@eks-distro-bot eks-distro-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 14, 2022
@g-gaston g-gaston force-pushed the spec-from-cluster branch 2 times, most recently from 168b38d to b4bf2a1 Compare June 14, 2022 16:23
@@ -131,6 +132,59 @@ func TestNewSpecValid(t *testing.T) {
validateSpecFromSimpleBundle(t, gotSpec)
}

func TestNewSpecFromClusterConfigTinkerbellValid(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I only added this to meet the coverage minimun (because the test was missing), I didn't touch this functionality

@@ -283,3 +337,46 @@ func TestBundlesRefDefaulter(t *testing.T) {
})
}
}

func TestBuildSpecFromBundles(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here, added for coverage diff min

}

// init does the basic initialization with the provided necessary api objects
func (s *Spec) init(config *Config, bundles *v1alpha1.Bundles, versionsBundle *v1alpha1.VersionsBundle, eksdRelease *eksdv1alpha1.Release) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

moved this piece to a separate method so it can be reused by NewSpecFromClusterConfig and BuildSpec


"github.com/aws/eks-anywhere/pkg/api/v1alpha1"
anywherev1 "github.com/aws/eks-anywhere/pkg/api/v1alpha1"
Copy link
Member Author

Choose a reason for hiding this comment

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

I did a bunch of package renames to follow our latest convention and because it was getting confusing with the old names

@@ -22,6 +22,9 @@ type EksdReleaseFetch func(ctx context.Context, name, namespace string) (*eksdv1

type OIDCFetch func(ctx context.Context, name, namespace string) (*v1alpha1.OIDCConfig, error)

// BuildSpec constructs a cluster.Spec for a eks-a cluster by retrieving all
// necessary objects using fetch methods
// This is deprecated in favour of BuildSpec
func BuildSpecForCluster(ctx context.Context, cluster *v1alpha1.Cluster, bundlesFetch BundlesFetch, eksdReleaseFetch EksdReleaseFetch, gitOpsFetch GitOpsFetch, fluxConfigFetch FluxConfigFetch, oidcFetch OIDCFetch) (*Spec, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We should get rid of this method now that we have the builder that uses a client. But I would leave that refactor for later to not blow up this PR

@g-gaston
Copy link
Member Author

/unhold

@eks-distro-bot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@g-gaston g-gaston force-pushed the spec-from-cluster branch from 68dd3d9 to 42e6a36 Compare June 14, 2022 20:03
@g-gaston g-gaston added the lgtm label Jun 14, 2022
@g-gaston
Copy link
Member Author

/approve

@eks-distro-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: g-gaston

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@eks-distro-bot eks-distro-bot merged commit 19348d0 into aws:main Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants