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

Integration test helpers rewrite #103

Merged
merged 6 commits into from
Jun 10, 2022

Conversation

sgettys
Copy link
Collaborator

@sgettys sgettys commented Jun 9, 2022

  • Update the integration tests to use the published agent image that includes the k8s plugin
  • Updated to 1.0-alpha.20
  • Consolidated integration test helpers using dynamic client. The reason for this is due to different concrete types that's used for the agent action status versus the porter resources status. Looked into different ways to solve this and dynamic client seems to be cleanest option without adding unnecessary interfaces or multiple abstraction layers

Steven Gettys added 5 commits June 8, 2022 11:25
…as k8s plugin included

Signed-off-by: Steven Gettys <s.gettys@f5.com>
Signed-off-by: Steven Gettys <s.gettys@f5.com>
Signed-off-by: Steven Gettys <s.gettys@f5.com>
Signed-off-by: Steven Gettys <s.gettys@f5.com>
Signed-off-by: Steven Gettys <s.gettys@f5.com>
@getporterbot getporterbot added this to In Progress in Porter and Mixins Jun 9, 2022
@codecov
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #103 (0c12f56) into main (38f717f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #103   +/-   ##
=======================================
  Coverage   78.07%   78.07%           
=======================================
  Files          12       12           
  Lines         976      976           
=======================================
  Hits          762      762           
  Misses        136      136           
  Partials       78       78           
Flag Coverage Δ
unit-tests 78.07% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
controllers/porter_resource.go 77.61% <ø> (ø)

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 38f717f...0c12f56. Read the comment docs.

Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

YES! This is so much better. Thanks for de-duping all that code.

@@ -636,18 +636,18 @@ func pwd() string {

// Run porter using the local storage, not the in-cluster storage
func buildPorterCmd(args ...string) shx.PreparedCommand {
mg.SerialDeps(porter.UseBinForPorterHome, porter.EnsurePorter)
mg.SerialDeps(porter.UseBinForPorterHome)
Copy link
Member

@carolynvs carolynvs Jun 10, 2022

Choose a reason for hiding this comment

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

This changes how often EnsurePorterAt is run. You can use an anonymous function (or just make a new private function called ensurePorterAt) to keep it as a mage target and retain the existing behavior.

Suggested change
mg.SerialDeps(porter.UseBinForPorterHome)
mg.SerialDeps(porter.UseBinForPorterHome, func(){porter.EnsurePorterAt(porterVersion)})


patchInstallation := func(inst *porterv1.Installation) {
controllers.PatchObjectWithRetry(ctx, logr.Discard(), k8sClient, k8sClient.Patch, inst, func() client.Object {
return &porterv1.Installation{}
})
// Wait for patch to apply, this can cause race conditions
Copy link
Member

Choose a reason for hiding this comment

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

I see a comment here but no corresponding code below. Does this comment apply to something I'm just missing or did you intend to add more waiting here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops this was before I figured out the race conditions in the waitForPorter, will remove

Expect(waitForPorter(ctx, inst, "waiting for porter-test-me to install")).Should(Succeed())
validateInstallationConditions(inst)
Expect(waitForPorter(ctx, inst, 1, "waiting for porter-test-me to install")).Should(Succeed())
validateResourceConditions(inst.Status.Conditions)
Copy link
Member

Choose a reason for hiding this comment

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

I think that you can take advantage of all of the resources implementing the porterResource interface. We can just start export it by renaming to PorterReource.

Then you can pass in an installation (not inst.Status.Conditions) and call GetStatus() which returns a struct with Conditions on it.

actionKey := client.ObjectKey{Name: name, Namespace: namespace}
action := &porterv1.AgentAction{}
if err := k8sClient.Get(ctx, actionKey, action); err != nil {
Log(errors.Wrap(err, "could not retrieve the Installation's AgentAction to troubleshoot").Error())
Copy link
Member

Choose a reason for hiding this comment

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

The error message here is specific to installation, should this say "Resource's AgentAction" instead?

jobKey := client.ObjectKey{Name: action.Status.Job.Name, Namespace: action.Namespace}
job := &batchv1.Job{}
if err := k8sClient.Get(ctx, jobKey, job); err != nil {
Log(errors.Wrap(err, "could not retrieve the Installation's Job to troubleshoot").Error())
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

actionKey := client.ObjectKey{Name: agentActionName, Namespace: namespace}
action := &porterv1.AgentAction{}
if err := k8sClient.Get(ctx, actionKey, action); err != nil {
Log(errors.Wrap(err, "could not retrieve the CredentialSet's AgentAction to troubleshoot").Error())
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Signed-off-by: Steven Gettys <s.gettys@f5.com>
Copy link
Member

@carolynvs carolynvs 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, thanks for cleaning up our copy/paste sins!

@carolynvs carolynvs merged commit efa428d into getporter:main Jun 10, 2022
Porter and Mixins automation moved this from In Progress to Done Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants