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

[org search] Adds both the display of and the ability to open search results URLs #13879

Merged
merged 7 commits into from
Sep 11, 2023

Conversation

kpitzen
Copy link
Contributor

@kpitzen kpitzen commented Sep 5, 2023

Description

Displays to users a copyable URL to view org search results in Pulumi Cloud as well as a --web flag to automatically open those results for them.

Fixes https://github.com/pulumi/pulumi.ai/issues/207

Checklist

  • I have run make tidy to update any new dependencies
  • I have run make lint to verify my code passes the lint check
    • I have formatted my code using gofumpt
  • I have added tests that prove my fix is effective or that my feature works
  • I have run make changelog and committed the changelog/pending/<file> documenting my change
  • Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version

@kpitzen kpitzen changed the title Kp/add service url for search Adds both the display of and the ability to open search results URLs Sep 5, 2023
@pulumi-bot
Copy link
Contributor

pulumi-bot commented Sep 5, 2023

Changelog

[uncommitted] (2023-09-11)

Features

  • [cli/config] Adheres to default org for org search when one is set
    #13879

  • [cli/display] Displays search URL when results table is rendered
    #13879

  • [cli/display] Displays total and displayed resource counts for org search
    #13879

@kpitzen kpitzen changed the title Adds both the display of and the ability to open search results URLs [org search] Adds both the display of and the ability to open search results URLs Sep 5, 2023
@kpitzen kpitzen marked this pull request as ready for review September 6, 2023 16:54
@@ -1147,6 +1147,10 @@ func getNaturalLanguageSearchPath(orgName string) string {
return fmt.Sprintf("/api/orgs/%s/search/resources/parse", url.PathEscape(orgName))
}

func getPulumiOrgSearchPath(orgName string) string {
return fmt.Sprintf("https://app.pulumi.com/%s/resources", url.PathEscape(orgName))
Copy link
Member

Choose a reason for hiding this comment

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

Should this not take the client address into account for self-hosted customers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should! I was actually unaware we had that information available here. Let me do some digging and plumb that in.

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 only place I could see this elsewhere was as passed as a CLI argument to --client. Is there another place this information is available? (Backend has the URL of the API we're interacting with, but this URL redirects to the front-end).

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeh I don't think we record the frontend, and doing that nicely probably requires service to give a new endpoint for us to call (because we only get the api url from the user when logging in).
Can you just add a TODO to that effect here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on @justinvp 's feedback below, it seems like backend's CloudConsoleURL method will do essentially exactly what we want here. Unless you believe that's incorrect, I'll go down that path for now.

pkg/cmd/pulumi/org_search.go Outdated Show resolved Hide resolved
pkg/cmd/pulumi/org_search.go Show resolved Hide resolved
Copy link
Member

@justinvp justinvp left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

Comment on lines 158 to 162
cmd.PersistentFlags().StringVar(
&scmd.client, "client", "https://app.pulumi.com",
"The base URL of the Pulumi Cloud service to direct to. This defaults to https://app.pulumi.com.",
)
_ = cmd.PersistentFlags().MarkHidden("client")
Copy link
Member

Choose a reason for hiding this comment

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

I would not accept this as a flag...

We have a way to convert an API URL to Console URL, which is used when running a pulumi up (or other operation) to show a permalink to the update in Pulumi Cloud. I would think we'd want to use the same approach here:

// getPermalink returns a link to the update in the Pulumi Console.
func (b *cloudBackend) getPermalink(update client.UpdateIdentifier, version int, preview bool) string {
base := b.cloudConsoleStackPath(update.StackIdentifier)
if !preview {
return b.CloudConsoleURL(base, "updates", strconv.Itoa(version))
}
return b.CloudConsoleURL(base, "previews", update.UpdateID)
}

// CloudConsoleURL returns a link to the cloud console with the given path elements. If a console link cannot be
// created, we return the empty string instead (this can happen if the endpoint isn't a recognized pattern).
func (b *cloudBackend) CloudConsoleURL(paths ...string) string {
return cloudConsoleURL(b.CloudURL(), paths...)
}

// cloudConsoleURL returns a URL to the Pulumi Cloud Console, rooted at cloudURL. If there is
// an error, returns "".
func cloudConsoleURL(cloudURL string, paths ...string) string {
u, err := url.Parse(cloudURL)
if err != nil {
return ""
}
switch {
case os.Getenv(ConsoleDomainEnvVar) != "":
// Honor a PULUMI_CONSOLE_DOMAIN environment variable to override the
// default behavior. Since we identify a backend by a single URI, we
// cannot know what the Pulumi Console is hosted at...
u.Host = os.Getenv(ConsoleDomainEnvVar)
case strings.HasPrefix(u.Host, defaultAPIDomainPrefix):
// ... but if the cloudURL (API domain) is "api.", then we assume the
// console is hosted at "app.".
u.Host = defaultConsoleDomainPrefix + u.Host[len(defaultAPIDomainPrefix):]
case u.Host == "localhost:8080":
// ... or when running locally, on port 3000.
u.Host = "localhost:3000"
default:
// We couldn't figure out how to convert the api hostname into a console hostname.
// We return "" so that the caller can know to omit the URL rather than just
// return an incorrect one.
return ""
}
u.Path = path.Join(paths...)
return u.String()
}

Copy link
Member

Choose a reason for hiding this comment

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

Ah forgot we had all this. Still would be nice to have an endpoint we could hit to be sure we've got the right url rather than the guesses in here. Something to ask service about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah this makes much more sense. I was going off the original feedback re: client address which only appears once as a flag:

func readProjectForUpdate(clientAddress string) (*workspace.Project, string, error) {

This makes significantly more sense to me. Thanks!

Additionally, adds the standard default behavior of showing help when required params are missing
<!---
Thanks so much for your contribution! If this is your first time
contributing, please ensure that you have read the
[CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md)
documentation.
-->

<!--- Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context. -->

Currently, `pulumi org search` will use the individual user account as
the default org when none is provided via `--org`. It would be ideal if
it also respected any org set by `pulumi org set-default`. This PR
accomplishes that.

Fixes https://github.com/pulumi/pulumi.ai/issues/208

- [x] I have run `make tidy` to update any new dependencies
- [x] I have run `make lint` to verify my code passes the lint check
  - [ ] I have formatted my code using `gofumpt`

<!--- Please provide details if the checkbox below is to be left
unchecked. -->
- [ ] I have added tests that prove my fix is effective or that my
feature works
<!---
User-facing changes require a CHANGELOG entry.
-->
- [x] I have run `make changelog` and committed the
`changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the
Pulumi Cloud,
then the service should honor older versions of the CLI where this
change would not exist.
You must then bump the API version in
/pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi
Cloud API version
<!-- @pulumi employees: If yes, you must submit corresponding changes in
the service repo. -->
@justinvp justinvp mentioned this pull request Sep 11, 2023
@kpitzen kpitzen added this pull request to the merge queue Sep 11, 2023
Merged via the queue into master with commit fe639f4 Sep 11, 2023
46 checks passed
@kpitzen kpitzen deleted the KP/AddServiceURLForSearch branch September 11, 2023 17:14
github-merge-queue bot pushed a commit that referenced this pull request Sep 12, 2023
I'll wait until #13879 has merged
before merging this.
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.

None yet

4 participants