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

[projects] remove configuration page from wizard #7102

Merged
merged 1 commit into from
Dec 8, 2021
Merged

Conversation

AlexTugarev
Copy link
Member

@AlexTugarev AlexTugarev commented Dec 7, 2021

instead of showing the configuration page, let's show a simple New Workspace button to start a workspace.

we rescue the auto-inferred configuration or use the existing one to trigger a prebuild right away.

many thanks to @jankeromnes to help get this done! 💯

Resolves #7098 and #7088

Screen Shot 2021-12-08 at 09 51 17

Projects: remove the configuration page from New Project wizard.

@jldec
Copy link
Contributor

jldec commented Dec 7, 2021

Please see also #7088 and #7098

@AlexTugarev
Copy link
Member Author

AlexTugarev commented Dec 7, 2021

/werft help

👍 You can interact with werft using: /werft command <args>.
Available commands are:

  • /werft run [annotation=value] which starts a new werft job from this context.
    You can optionally pass multiple whitespace-separated annotations.
  • /werft help displays this help

@jldec
Copy link
Contributor

jldec commented Dec 7, 2021

comments on the current implementation

  • The link in the text at the top goes to /new
  • The text below the button is a bit confusing
    not all users will understand that it explains what the button is going to do.

@AlexTugarev
Copy link
Member Author

I had to redeploy the whole thing. Workspaces failed to start after a rebase, unfortunately.

@AlexTugarev
Copy link
Member Author

AlexTugarev commented Dec 7, 2021

/werft run

👍 started the job as gitpod-build-at-config-page.8

@AlexTugarev AlexTugarev force-pushed the at/config-page branch 2 times, most recently from bce4aa2 to 33734f0 Compare December 7, 2021 16:30
@jldec
Copy link
Contributor

jldec commented Dec 7, 2021

I think for consistency, and to help users, we need 1 line of explanatory text (in smaller, lighter font) under the project created. @gtsiolis would you mind taking a look, and letting us know what you think?

Otherwise I think this resolves both #7088 and #7098

@jldec
Copy link
Contributor

jldec commented Dec 7, 2021

Another idea - with links to project and team (project list)

Screenshot 2021-12-07 at 18 40 23

@AlexTugarev
Copy link
Member Author

@jldec, it's updated, and build is triggered. Please note, workspaces do fail to start on preview environments at the moment. That's nothing to be part of this PR.

Comment on lines +99 to +104
setSourceOfConfig("db");
setGuessedConfigString(await guessedConfigStringPromise || `tasks:
- init: |
echo 'TODO: build project'
command: |
echo 'TODO: start app'`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there can be a race here -- if await guessedConfigStringPromise takes too long, you might call server.setProjectConfiguration(project.id, undefined) on line 154.

Safer the other way around:

Suggested change
setSourceOfConfig("db");
setGuessedConfigString(await guessedConfigStringPromise || `tasks:
- init: |
echo 'TODO: build project'
command: |
echo 'TODO: start app'`);
setGuessedConfigString(await guessedConfigStringPromise || `tasks:
- init: |
echo 'TODO: build project'
command: |
echo 'TODO: start app'`);
setSourceOfConfig("db");

Comment on lines 501 to 502
<p className="mt-2 text-gray-500 text-center text-base">Created <a className="gp-link" href={projectLink}>{project.name}</a>
<br /> {location}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This deviates slightly from the spec, but I'd find it a lot better to explicitly state what's a "project" and what's a "team", just to avoid any possible confusion here (as we've seen in user testing, users are sometimes confused between workspace vs team vs project, so might as well be 100% clear here).

Also, the new line between the two doesn't make sense, and actually looks like a UI bug:

Suggested change
<p className="mt-2 text-gray-500 text-center text-base">Created <a className="gp-link" href={projectLink}>{project.name}</a>
<br /> {location}
<p className="mt-2 text-gray-500 text-center text-base">Created project <a className="gp-link" href={projectLink}>{project.name}</a> {location}

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@jankeromnes jankeromnes Dec 8, 2021

Choose a reason for hiding this comment

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

To me, spec = visuals added to an issue that show what we want to have.

Here, it's #7098 (see "refined visual" at the bottom of the issue description)

However, I slightly disagree with the spec, because I think:

Project Created
Created project [test-project-2] in team [team-1]

is clearer than:

Project created
Created [test-project-2]
in [team-1]

@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

Merging #7102 (baa41ec) into main (34e4dcb) will increase coverage by 12.42%.
The diff coverage is n/a.

❗ Current head baa41ec differs from pull request most recent head 9030481. Consider uploading reports for the commit 9030481 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##             main    #7102       +/-   ##
===========================================
+ Coverage   19.04%   31.47%   +12.42%     
===========================================
  Files           2       28       +26     
  Lines         168     4823     +4655     
===========================================
+ Hits           32     1518     +1486     
- Misses        134     3187     +3053     
- Partials        2      118      +116     
Flag Coverage Δ
components-local-app-app-linux-amd64 19.04% <0.00%> (ø)
components-local-app-app-linux-arm64 ∅ <0.00%> (∅)
components-local-app-app-windows-386 ∅ <0.00%> (∅)
components-local-app-app-windows-amd64 ∅ <0.00%> (∅)
components-local-app-app-windows-arm64 ∅ <0.00%> (∅)
components-ws-manager-app 40.62% <0.00%> (?)
installer-raw-app 5.76% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
installer/pkg/components/ws-manager/tlssecret.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/manager/create.go 82.98% <0.00%> (ø)
installer/pkg/common/objects.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/manager/probe.go 0.00% <0.00%> (ø)
installer/pkg/components/ws-manager/rolebinding.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/manager/manager_ee.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/clock/clock.go 68.62% <0.00%> (ø)
installer/pkg/common/common.go 4.64% <0.00%> (ø)
installer/pkg/components/ws-manager/configmap.go 29.71% <0.00%> (ø)
components/ws-manager/pkg/manager/monitor.go 9.46% <0.00%> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34e4dcb...9030481. Read the comment docs.

instead of showing the configuration page, let's show a simple `New Workspace` button to start a workspace.

we rescue the auto-inferred configuration or use the existing one to trigger a prebuild right away.

Co-authored-by: Jan Keromnes <[email protected]>
Co-authored-by: Alex Tugarev <[email protected]>
@jldec
Copy link
Contributor

jldec commented Dec 8, 2021

put some comments in #7098 (comment)
but happy to push this forward now - thanks @AlexTugarev and @jankeromnes

/lgtm

@roboquat
Copy link
Contributor

roboquat commented Dec 8, 2021

LGTM label has been added.

Git tree hash: a17be6f3e98c535a7f152f53898151565c09af6c

@roboquat
Copy link
Contributor

roboquat commented Dec 8, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jldec

Associated issue: #7098

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit a481e8b into main Dec 8, 2021
@roboquat roboquat deleted the at/config-page branch December 8, 2021 16:13
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note size/L team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New project created page
4 participants