forcing the werft job to fail if the branch name is too long to suppo… #8316

fullmetalrooster · 2022-02-18T15:00:06Z

/werft no-preview

Description

This set a limit for the branch name to 26 characters to make sure that preview-environments can get started. The length depends either on the length of the domain that is created or the label that is added by ws-daemon/registry-facade.
It does not solve the problem but at least gives a useful information instead of just failing.

Related Issue(s)

Fixes #8169

How to test

Run a preview environment on a branch with a long, 26 character at least, and a short name.

Release Notes

None

Documentation

codecov · 2022-02-18T15:03:18Z

Codecov Report

Merging #8316 (2af29f0) into main (6f8bbba) will increase coverage by 2.75%.
The diff coverage is n/a.

❗ Current head 2af29f0 differs from pull request most recent head d78dad7. Consider uploading reports for the commit d78dad7 to get more accurate results

@@            Coverage Diff            @@
##            main    #8316      +/-   ##
=========================================
+ Coverage   8.42%   11.17%   +2.75%     
=========================================
  Files         33       18      -15     
  Lines       2339      993    -1346     
=========================================
- Hits         197      111      -86     
+ Misses      2137      880    -1257     
+ Partials       5        2       -3

Flag	Coverage Δ
components-gitpod-cli-app	`11.17% <ø> (ø)`
components-installer-raw-app	`?`
components-local-app-app-darwin-amd64	`?`
components-local-app-app-darwin-arm64	`?`
components-local-app-app-linux-amd64	`?`
components-local-app-app-linux-arm64	`?`
components-local-app-app-windows-386	`?`
components-local-app-app-windows-amd64	`?`
components-local-app-app-windows-arm64	`?`

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

Impacted Files	Coverage Δ
components/installer/pkg/common/objects.go
.../installer/pkg/components/ws-manager/deployment.go
components/installer/pkg/common/common.go
...components/ws-manager/unpriviledged-rolebinding.go
components/installer/pkg/common/ca.go
...s/installer/pkg/components/ws-manager/tlssecret.go
...staller/pkg/components/ws-manager/networkpolicy.go
components/local-app/pkg/auth/auth.go
components/installer/pkg/common/render.go
components/local-app/pkg/auth/pkce.go
... and 5 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 6f8bbba...d78dad7. Read the comment docs.

.werft/jobs/build/deploy-to-preview-environment.ts

csweichel · 2022-02-21T11:33:57Z

.werft/jobs/build/deploy-to-preview-environment.ts

+    // echo -n "gitpod.io/registry-facade_ready_ns_staging-" | wc -c
+    const maxBranchNameLength = 20;
+    if (deploymentConfig.destname.length > maxBranchNameLength) {
+        werft.fail(phases.PREDEPLOY, `The branch name ${deploymentConfig.destname} is more than ${maxBranchNameLength} character. Please choose a shorter name!`)


We should fail much sooner than that - best before we do anything.
Note: don't forget to tie this to the absence of no-preview

I put it there as it is the phase where it would break. I created an extra phase that fails early. I tested it with this branch [1], with noPreview [2] and with a branch that has a shorter name [3].

corneliusludmann · 2022-02-21T13:38:06Z

Can't we just shorten the destname to the required length instead of letting the job fail? E.g. here:

gitpod/.werft/jobs/build/job-config.ts

Lines 88 to 93 in 27b614e

    
           const previewDestName = version.split(".")[0]; 
        
           const previewEnvironmentNamespace = withVM ? `default` : `staging-${previewDestName}`; 
        
           const previewEnvironment = { 
        
               destname: previewDestName, 
        
               namespace: previewEnvironmentNamespace 
        
           }

geropl · 2022-02-21T16:32:00Z

Can't we just shorten the destname to the required length instead of letting the job fail?

IMO this is just a quick fix, and we should merge the early-as-possible error fix here. Hopefully we won't have this restriction too long, until we can use 1 VM / preview environment.

fullmetalrooster · 2022-02-21T17:53:09Z

Can't we just shorten the destname to the required length instead of letting the job fail? E.g. here:

gitpod/.werft/jobs/build/job-config.ts

Lines 88 to 93 in 27b614e

const previewDestName = version.split(".")[0];

const previewEnvironmentNamespace = withVM ? `default` : `staging-${previewDestName}`;

const previewEnvironment = {

destname: previewDestName,

namespace: previewEnvironmentNamespace

}

I do not know all places where the branch-name is used and simply shortening it might break something down the line.

mads-hartmann

Thanks @wulfthimm!

Let's get this merged - we're working on allowing longer names in parallel - follow https://github.com/gitpod-io/ops/issues/1252 if you're interested in that.

mads-hartmann · 2022-02-24T13:38:01Z

@wulfthimm Adding the hold label - the build job needs to be happy before this can be merged ☺️

The base branch was changed.

fullmetalrooster · 2022-02-24T14:09:25Z

@wulfthimm Adding the hold label - the build job needs to be happy before this can be merged relaxed

I added the no-preview flag. And now it was successful. For verification I started a job with this PR on a shorter branch. https://werft.gitpod-dev.com/job/gitpod-build-wth-short.0

mads-hartmann

@wulfthimm I approved but added the hold label. Please merge this PR manually using Squash and merge so you can edit the commit message to get rid of the /werft command (otherwise branches off main won't get a preview environment)

mads-hartmann · 2022-02-24T14:15:19Z

@wulfthimm hah, okay, tide is a required check so you can't merge manually 😂 You have to

Put /werft no-preview in your PR description
Reword your latest commit to not have the /werft command
Force push

Then I can approve and Tide can merge it.

…rt preview-environments

fullmetalrooster · 2022-02-24T14:23:57Z

@mads-hartmann It is ready now.

csweichel · 2022-02-25T11:24:59Z

@mads-hartmann someone would need to unhold if this should be merged

roboquat added do-not-merge/release-note-label-needed size/XS labels Feb 18, 2022

roboquat added release-note-none and removed do-not-merge/release-note-label-needed labels Feb 18, 2022

geropl reviewed Feb 18, 2022

View reviewed changes

.werft/jobs/build/deploy-to-preview-environment.ts Outdated Show resolved Hide resolved

fullmetalrooster force-pushed the wulfthimm/max-branch-name-lenght-is-toooooooo-long branch from f05e1d4 to e666dfe Compare February 21, 2022 07:44

fullmetalrooster requested review from jldec and geropl February 21, 2022 07:46

mads-hartmann mentioned this pull request Feb 21, 2022

[build] make werft fail early when the branch name is too long #8304

Closed

csweichel reviewed Feb 21, 2022

View reviewed changes

roboquat added size/S size/XS and removed size/XS size/S labels Feb 22, 2022

fullmetalrooster force-pushed the wulfthimm/max-branch-name-lenght-is-toooooooo-long branch 3 times, most recently from 0573ed4 to 3ed4306 Compare February 22, 2022 07:42

fullmetalrooster requested a review from csweichel February 22, 2022 07:42

ArthurSens mentioned this pull request Feb 24, 2022

werft/job-config: Make previews from 20+ character branches possible #8403

Closed

mads-hartmann previously approved these changes Feb 24, 2022

View reviewed changes

mads-hartmann added the do-not-merge/hold label Feb 24, 2022

fullmetalrooster changed the base branch from main to wth/test February 24, 2022 13:44

fullmetalrooster changed the base branch from wth/test to main February 24, 2022 13:44

fullmetalrooster mentioned this pull request Feb 24, 2022

Forcing the werft job to fail if the branch name is too long to support preview-environments #8433

Closed

fullmetalrooster force-pushed the wulfthimm/max-branch-name-lenght-is-toooooooo-long branch from 3ed4306 to da707ac Compare February 24, 2022 14:02

fullmetalrooster requested a review from mads-hartmann February 24, 2022 14:09

fullmetalrooster removed the do-not-merge/hold label Feb 24, 2022

mads-hartmann added the do-not-merge/hold label Feb 24, 2022

mads-hartmann previously approved these changes Feb 24, 2022

View reviewed changes

forcing the werft job to fail if the branch name is too long to suppo…

d78dad7

…rt preview-environments

fullmetalrooster dismissed mads-hartmann’s stale review via d78dad7 February 24, 2022 14:17

fullmetalrooster force-pushed the wulfthimm/max-branch-name-lenght-is-toooooooo-long branch from da707ac to d78dad7 Compare February 24, 2022 14:17

fullmetalrooster requested a review from mads-hartmann February 24, 2022 14:26

mads-hartmann approved these changes Feb 24, 2022

View reviewed changes

fullmetalrooster removed the do-not-merge/hold label Feb 25, 2022

roboquat merged commit 73cbeef into main Feb 25, 2022

roboquat deleted the wulfthimm/max-branch-name-lenght-is-toooooooo-long branch February 25, 2022 11:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

forcing the werft job to fail if the branch name is too long to suppo… #8316

forcing the werft job to fail if the branch name is too long to suppo… #8316

fullmetalrooster commented Feb 18, 2022 •

edited

Loading

codecov bot commented Feb 18, 2022 •

edited

Loading

csweichel Feb 21, 2022

geropl Feb 21, 2022

fullmetalrooster Feb 22, 2022

corneliusludmann commented Feb 21, 2022

geropl commented Feb 21, 2022

fullmetalrooster commented Feb 21, 2022 •

edited

Loading

mads-hartmann left a comment

mads-hartmann commented Feb 24, 2022 •

edited by werft-gitpod-dev-com bot

Loading

fullmetalrooster commented Feb 24, 2022

mads-hartmann left a comment

mads-hartmann commented Feb 24, 2022 •

edited by werft-gitpod-dev-com bot

Loading

fullmetalrooster commented Feb 24, 2022

csweichel commented Feb 25, 2022

forcing the werft job to fail if the branch name is too long to suppo… #8316

forcing the werft job to fail if the branch name is too long to suppo… #8316

Conversation

fullmetalrooster commented Feb 18, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

codecov bot commented Feb 18, 2022 • edited Loading

Codecov Report

csweichel Feb 21, 2022

Choose a reason for hiding this comment

geropl Feb 21, 2022

Choose a reason for hiding this comment

fullmetalrooster Feb 22, 2022

Choose a reason for hiding this comment

corneliusludmann commented Feb 21, 2022

geropl commented Feb 21, 2022

fullmetalrooster commented Feb 21, 2022 • edited Loading

mads-hartmann left a comment

Choose a reason for hiding this comment

mads-hartmann commented Feb 24, 2022 • edited by werft-gitpod-dev-com bot Loading

fullmetalrooster commented Feb 24, 2022

mads-hartmann left a comment

Choose a reason for hiding this comment

mads-hartmann commented Feb 24, 2022 • edited by werft-gitpod-dev-com bot Loading

fullmetalrooster commented Feb 24, 2022

csweichel commented Feb 25, 2022

fullmetalrooster commented Feb 18, 2022 •

edited

Loading

codecov bot commented Feb 18, 2022 •

edited

Loading

fullmetalrooster commented Feb 21, 2022 •

edited

Loading

mads-hartmann commented Feb 24, 2022 •

edited by werft-gitpod-dev-com bot

Loading

mads-hartmann commented Feb 24, 2022 •

edited by werft-gitpod-dev-com bot

Loading