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

feat: update destination add to use api endpoint #3058

Merged
merged 3 commits into from
Sep 8, 2022
Merged

Conversation

mxyng
Copy link
Collaborator

@mxyng mxyng commented Aug 29, 2022

Summary

  • Use api.{baseDomain} when templating destination add command
  • Use values file instead of --set
  • Use upgrade --install instead of install
  • Make baseDomain required when signup is enabled

image

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

The Go changes LGTM, but I notice there is one test failing that needs to be updated for this new validation.

I guess using a heredoc with a values.yaml file should make the command easier to copy.

@@ -2,8 +2,10 @@ import Head from 'next/head'
import Link from 'next/link'
import { useState, useEffect } from 'react'
import copy from 'copy-to-clipboard'
import yaml from 'js-yaml'
Copy link
Contributor

Choose a reason for hiding this comment

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

JSON is valid yaml, so we could potentially avoid this dependency by using JSON.stringify() and writing it out as JSON.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could do that but IMO JSON is less user friendly with its quotes and braces and brackets. I don't think it's worth using JSON solely to avoid this dependency.

}

const valuesYaml = yaml.dump(values)
const command = `cat <<EOF | helm upgrade --install infra-connector infrahq/infra -f-\n${valuesYaml}EOF`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just do this all with flags? The cat piping to get multi-line input looks confusing imo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could. It would look like the previous command snippet. IMO it's actually harder to read and understand. Flags simulate a hierarchy which can easily be overlooked. This makes the relationship between the values clear while also making it easier to edit or append.

Additionally, the settings doesn't translate directly to a values file so if the user wants to adds the outputs to IAC, they'll need to transcribe it into YAML. This way, we can add an option to download the file instead of using a heredoc and the helm command will remain the same.

@mxyng mxyng force-pushed the mxyng/update-dest-add branch 5 times, most recently from 8fd6a31 to a8184ba Compare September 2, 2022 22:26
@mxyng mxyng requested a review from BruceMacD September 6, 2022 16:34
@mxyng mxyng merged commit fbe1bc1 into main Sep 8, 2022
@mxyng mxyng deleted the mxyng/update-dest-add branch September 8, 2022 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants