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

Do not deploy runtime chart if runtime is not needed DEV-1225 #12

Merged
merged 3 commits into from
Nov 9, 2022

Conversation

LucilleH
Copy link
Collaborator

@LucilleH LucilleH commented Nov 8, 2022

Summary

If runtime is not needed, do not deploy runtime helm chart

How was it tested?

launchpad up

Is this change backwards-compatible?

Yes

@mikeland73
Copy link
Contributor

Take a look at https://github.com/jetpack-io/launchpad/blob/main/launchpad/deploy.go#L312 which is already skipping the runtime. It looks like we're taking to slightly different approaches, maybe there's a way to unify?

@LucilleH
Copy link
Collaborator Author

LucilleH commented Nov 8, 2022

Take a look at https://github.com/jetpack-io/launchpad/blob/main/launchpad/deploy.go#L312 which is already skipping the runtime. It looks like we're taking to slightly different approaches, maybe there's a way to unify?

Oh good point. Updated.

The previous way have a downside of downloading and generating the information needed for the runtime chart which requires user information, even when runtime is not needed. The current way avoids this step if runtime is not needed.

I think we still need the runSDKRegistration bool for the helm chart values so I'll keep that, and that is also derived from the same detect.Sdk function. So I think now it is unified. Thoughts? @mikeland86

@LucilleH LucilleH requested a review from ipince November 9, 2022 17:27
@@ -92,6 +92,8 @@ func makeDeployOptions(
if err != nil {
return nil, err
}

var runtimeHelm *launchpad.HelmOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

move to line 112 (closer to its assignment)?

@LucilleH LucilleH merged commit 487cb84 into main Nov 9, 2022
@LucilleH LucilleH deleted the lucille--auth-helm branch November 9, 2022 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants