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

app/obolapi: create cluster launchpad URL #2518

Merged
merged 4 commits into from
Aug 8, 2023

Conversation

gsora
Copy link
Collaborator

@gsora gsora commented Aug 7, 2023

Add the LaunchpadURLForLock method, which generates a Launchpad cluster dashboard URL for a given cluster lock.

Reference this method in dkg and create cluster commands so that at the end of those processes, if the user has specified --publish, the Launchpad dashboard URL will be printed on the console.

category: feature
ticket: #2423

Closes #2423.

Add the `LaunchpadURLForLock` method, which generates a Launchpad cluster dashboard URL for a given cluster lock.

Reference this method in `dkg` and `create cluster` commands so that at the end of this processes, if the user has specified `--publish`, the Launchpad dashboard URL will be printed on the console.
@gsora gsora self-assigned this Aug 7, 2023
@gsora
Copy link
Collaborator Author

gsora commented Aug 7, 2023

Solving merge conflicts rn.

@gsora
Copy link
Collaborator Author

gsora commented Aug 7, 2023

Done, ready for review.

Needs input from @f1lander / @HananINouman / @OisinKyne for URL format (see Slack) and general UX.

@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Patch coverage: 71.69% and project coverage change: +0.03% 🎉

Comparison is base (d35f366) 53.70% compared to head (6347970) 53.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2518      +/-   ##
==========================================
+ Coverage   53.70%   53.74%   +0.03%     
==========================================
  Files         199      199              
  Lines       26908    26938      +30     
==========================================
+ Hits        14451    14477      +26     
- Misses      10659    10661       +2     
- Partials     1798     1800       +2     
Files Changed Coverage Δ
cmd/createcluster.go 59.82% <52.94%> (-0.24%) ⬇️
dkg/dkg.go 57.12% <61.53%> (+<0.01%) ⬆️
app/obolapi/api.go 65.51% <91.30%> (+17.69%) ⬆️

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gsora
Copy link
Collaborator Author

gsora commented Aug 7, 2023

Screenshot 2023-08-07 at 11 35 41

For reference, this is how it appears to users when a DKG with --publish completes.

/cc @Battenfield

if err != nil {
return errors.Wrap(err, "invalid endpoint")
}
addr := *c.baseURL
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep baseURL a string since this pointer trick is easy to miss when adding more functions which can result in bugs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you think about this? Validate url in New(), and provide a url() method that returns it as *url.URL, panic-ing if parsing fails: since validation happens in New(), if it fails later it's a very weird situation:

// New returns a new Client.
func New(urlStr string) (Client, error) {
	_, err := url.ParseRequestURI(urlStr) // check that urlStr is valid
	if err != nil {
		return Client{}, errors.Wrap(err, "could not parse Obol API URL")
	}

	return Client{
		baseURL: urlStr,
	}, nil
}

// Client is the REST client for obol-api requests.
type Client struct {
	baseURL string // Base obol-api URL
}

func (c Client) url() *url.URL {
	baseURL, err := url.ParseRequestURI(c.baseURL)
	if err != nil {
		panic(errors.Wrap(err, "could not parse Obol API URL, this should never happen"))
	}

	return baseURL
}

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, could also just return the error from url(), don't think the effort to omit errors is super valuable.

app/obolapi/api.go Outdated Show resolved Hide resolved
cmd/createcluster.go Outdated Show resolved Hide resolved
@gsora gsora added the merge when ready Indicates bulldozer bot may merge when all checks pass label Aug 8, 2023
@obol-bulldozer obol-bulldozer bot merged commit 6e07bf8 into main Aug 8, 2023
@obol-bulldozer obol-bulldozer bot deleted the gsora/dkg_publish_launchpad_url branch August 8, 2023 14:31
gsora added a commit that referenced this pull request Aug 9, 2023
Add the `LaunchpadURLForLock` method, which generates a Launchpad cluster dashboard URL for a given cluster lock.

Reference this method in `dkg` and `create cluster` commands so that at the end of those processes, if the user has specified `--publish`, the Launchpad dashboard URL will be printed on the console.

category: feature
ticket: #2423 

Closes #2423.
gsora added a commit that referenced this pull request Aug 9, 2023
Add the `LaunchpadURLForLock` method, which generates a Launchpad cluster dashboard URL for a given cluster lock.

Reference this method in `dkg` and `create cluster` commands so that at the end of those processes, if the user has specified `--publish`, the Launchpad dashboard URL will be printed on the console.

category: feature
ticket: #2423 

Closes #2423.
obol-bulldozer bot pushed a commit that referenced this pull request Aug 9, 2023
The following PRs have been cherry-picked:

- #2490
- #2494
- #2496
- #2509
- #2510
- #2504
- #2511
- #2514
- #2516
- #2518


category: misc
ticket: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Launchpad URL Return for --publish Command
2 participants