Skip to content

Layer ordering #364

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

Merged
merged 6 commits into from
Dec 7, 2021
Merged

Layer ordering #364

merged 6 commits into from
Dec 7, 2021

Conversation

bigkevmcd
Copy link
Contributor

This PR implements ordering of the generated HelmReleases.

Profile helm charts can be annotated with weave.works/layer: layer-name.

Charts with metadata annotations are ordered based on a sorted version of the layers.

For example, if two Profile charts are being installed in layer-0 and layer-1, the charts with declared layer-1 will be configured as dependsOn the chart in layer-0.

@bigkevmcd bigkevmcd added the enhancement New feature or request label Dec 1, 2021
Copy link
Contributor

@palemtnrider palemtnrider left a comment

Choose a reason for hiding this comment

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

A few suggested changes but other than that this LGTM.

Name: helmRepo.ObjectMeta.Name,
Namespace: helmRepo.ObjectMeta.Namespace,

var names []string
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - the variable name 'names' took me a minute to realize this was the name of the layers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there's a lot going on in this function, I've renamed it to layerNames.

},
TypeMeta: metav1.TypeMeta{
APIVersion: helmv2beta1.GroupVersion.Identifier(),
Kind: helmv2beta1.HelmReleaseKind,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on adding a label with the layer value? It will be useful to be able to select all HRs for a particular layer.

}
}

sort.Slice(releases, func(i, j int) bool { return releases[i].GetName() < releases[j].GetName() })
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add some details on the sort here? Based on the function I would expect the sort to be layer, name instead of just name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sort here is just used to make testing easier, the generated HelmReleases would be applied "simultaneously" because they're written to the same file.

}

helmReleases, err := charts.MakeHelmReleasesInLayers(clusterName, "wego-system", installs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Future work - we need to enable a namespace other than wego-system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used wego-system because that's what the original mechanism was using, but definitely yes.

I was trying to ensure that this was a drop-in replacement for the existing code :-)

@bigkevmcd bigkevmcd force-pushed the layer-ordering branch 2 times, most recently from 5e919f5 to 86f5034 Compare December 2, 2021 10:08
@bigkevmcd bigkevmcd requested a review from yiannistri December 3, 2021 10:08
Copy link
Collaborator

@yiannistri yiannistri left a comment

Choose a reason for hiding this comment

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

Looks great 🥇

@@ -1220,7 +1302,7 @@ func createServer(clusterState []runtime.Object, configMapName, namespace string
}

func createDatabase(t *testing.T) *gorm.DB {
db, err := utils.OpenDebug("", true)
db, err := utils.OpenDebug("", os.Getenv("DEBUG_SERVER_DB") == "true")
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️ 🙏

bigkevmcd and others added 6 commits December 7, 2021 14:28
Empty layer dependency.

Rework to simplify the creation without having to have Charts.

Update the protobuf to accept the layer.

Fix the rename function failure.
Don't debug log from database.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants