Skip to content

Connect: Add tests for ParseClusterURI#15863

Merged
ravicious merged 5 commits into
masterfrom
ravicious/uri-tests
Aug 30, 2022
Merged

Connect: Add tests for ParseClusterURI#15863
ravicious merged 5 commits into
masterfrom
ravicious/uri-tests

Conversation

@ravicious
Copy link
Copy Markdown
Member

I was working on a new feature and at some point I needed to extract a cluster URI from a resource URI. I wasn't sure if uri.ParseClusterURI behaves the way I expect it to so I decided to add some tests.

As this change is unrelated to the actual feature I'm working on, I decided to make a separate PR to keep the other PR smaller.

If you're not familiar with Connect's URIs, the readme has a quick overview of them.

Copy link
Copy Markdown
Contributor

@AntonAM AntonAM left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment thread lib/teleterm/api/uri/uri_test.go Outdated
Comment thread lib/teleterm/api/uri/uri_test.go Outdated
{
"/clusters/cluster.sh",
uri.NewClusterURI("cluster.sh"),
}, {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think gofmt might need running on this file :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hmm I'm running gofmt on save and running it manually gofmt -d api/utils/uri_test.go doesn't return anything. 🤔

Before the last commit line 64 was }{{ instead of just }{ and I changed it there.

Copy link
Copy Markdown
Member Author

@ravicious ravicious Aug 30, 2022

Choose a reason for hiding this comment

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

It might be weird handling of tabs by GitHub?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh ok, maybe I'm confusing gofmt with something I've got installed, but I'd have expected line 68 to match lines 71 and 75.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, I see, that was probably a leftover of that change from }{{ to }{.

@ravicious ravicious enabled auto-merge (squash) August 30, 2022 09:37
@ravicious ravicious merged commit 9397d4a into master Aug 30, 2022
@github-actions
Copy link
Copy Markdown
Contributor

@ravicious See the table below for backport results.

Branch Result
branch/v10 Create PR

@ravicious ravicious deleted the ravicious/uri-tests branch August 30, 2022 12:05
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.

3 participants