wip: fix: Get Velero Azure plugin operational #7451

chrismellard · 2020-07-18T10:54:38Z

Submitter checklist

Change is code complete and matches issue description.
Change is covered by existing or new tests.

Description

Adds in new requirementsConfig keys to support Azure Velero plugin. Further changes required to be made to boot repo. Need to implement part of the BucketProvider interface to get through the boot process. Only those methods required to get through jx boot implemented for now

Special notes for the reviewer(s)

Which issue this PR fixes

fixes #7450

jenkins-x-bot · 2020-07-18T10:54:41Z

Hi @chrismellard. Thanks for your PR.

I'm waiting for a jenkins-x member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository.

jenkins-x-bot · 2020-07-18T10:55:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign pmuir
You can assign the PR to them by writing /assign @pmuir in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

chrismellard · 2020-07-18T11:01:48Z

Related jenkins-x-boot-config PR here jenkins-x/jenkins-x-boot-config#168

pkg/cloud/aks/storage/bucket_provider.go

pkg/cloud/aks/aks.go

pkg/cloud/aks/storage/bucket_provider.go

pkg/cloud/aks/aks.go

ankitm123 · 2020-07-18T14:27:23Z

I know the codebase does not have good test coverage, but would be nice if you can add some tests for the code u wrote.
If you are ok with adding tests later, I am cool with that :), may be a different PR (once you get more familiar with the code base). Adding new features is always great, but without tests we run the risk of introducing regression.

ankitm123 · 2020-07-19T01:42:12Z

Also, you should squash your commits, and run make lint, I see some goimport issues :)

chrismellard · 2020-07-19T02:08:51Z

Thanks. Left the two commits in there as they were addressing different concerns - kinda. But happy to squash them up with addressing your feedback. Try get something turned around tonight.

ankitm123 · 2020-07-19T11:46:31Z

/ok-to-test

pkg/cloud/aks/storage/bucket_provider.go

pkg/cloud/aks/storage/bucket_provider_test.go

pkg/cloud/aks/mocks/azurestorage.go

rawlingsj · 2020-07-21T09:24:46Z

/retest

rawlingsj · 2020-07-21T12:55:07Z

/retest

ankitm123 · 2020-07-22T00:33:13Z

I think you should rebase this PR with the latest master, there was a PR which fixed issues with builds: #7460 @chris-mellard

chrismellard · 2020-07-22T01:28:13Z

🤷

pkg/gits/errors.go

rawlingsj · 2020-07-24T11:15:11Z

this is the unit test failure:

--- FAIL: TestNoForkAndExistingDir (0.20s)
    helpers_test.go:1137: 
        	Error Trace:	helpers_test.go:1137
        	            				helpers_test.go:385
        	Error:      	Received unexpected error:
        	            	failed to run 'git cherry-pick 9492c14c9b08ddd9f2b1230fc486fb154955bd48' command in directory '/tmp/177040746', output: 'The previous cherry-pick is now empty, possibly due to conflict resolution.
        	            	If you wish to commit it anyway, use:
        	            	
        	            	    git commit --allow-empty
        	            	
        	            	Otherwise, please use 'git reset'
        	            	On branch master
        	            	You are currently cherry-picking commit 9492c14.
        	            	
        	            	nothing to commit, working tree clean'
        	            	git output: The previous cherry-pick is now empty, possibly due to conflict resolution.
        	            	If you wish to commit it anyway, use:
        	            	
        	            	    git commit --allow-empty
        	            	
        	            	Otherwise, please use 'git reset'
        	            	On branch master
        	            	You are currently cherry-picking commit 9492c14.
        	            	
        	            	nothing to commit, working tree clean
        	            	github.com/jenkins-x/**/v2/pkg/gits.(*GitCLI).gitCmd
        	            		/workspace/source/pkg/gits/git_cli.go:598
        	            	github.com/jenkins-x/**/v2/pkg/gits.(*GitCLI).CherryPick
        	            		/workspace/source/pkg/gits/git_cli.go:1246
        	            	github.com/jenkins-x/**/v2/pkg/gits.ForkAndPullRepo
        	            		/workspace/source/pkg/gits/helpers.go:681
        	            	github.com/jenkins-x/**/v2/pkg/gits_test.runForkAndPullTestCase
        	            		/workspace/source/pkg/gits/helpers_test.go:1130
        	            	github.com/jenkins-x/**/v2/pkg/gits_test.TestNoForkAndExistingDir
        	            		/workspace/source/pkg/gits/helpers_test.go:385
        	            	testing.tRunner
        	            		/usr/local/go/src/testing/testing.go:909
        	            	runtime.goexit
        	            		/usr/local/go/src/runtime/asm_amd64.s:1357
        	            	Unable to cherry-pick 9492c14c9b08ddd9f2b1230fc486fb154955bd48
        	            	github.com/jenkins-x/**/v2/pkg/gits.ForkAndPullRepo
        	            		/workspace/source/pkg/gits/helpers.go:690
        	            	github.com/jenkins-x/**/v2/pkg/gits_test.runForkAndPullTestCase
        	            		/workspace/source/pkg/gits/helpers_test.go:1130
        	            	github.com/jenkins-x/**/v2/pkg/gits_test.TestNoForkAndExistingDir
        	            		/workspace/source/pkg/gits/helpers_test.go:385
        	            	testing.tRunner
        	            		/usr/local/go/src/testing/testing.go:909
        	            	runtime.goexit
        	            		/usr/local/go/src/runtime/asm_amd64.s:1357
        	Test:       	TestNoForkAndExistingDir
    helpers_test.go:1141: 
        	Error Trace:	helpers_test.go:1141
        	            				helpers_test.go:385
        	Error:      	Not equal: 
        	            	expected: "/tmp/177040746"
        	            	actual  : ""
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-/tmp/177040746
        	            	+
        	Test:       	TestNoForkAndExistingDir
    helpers_test.go:1142: 
        	            	actual  : ""
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-master
        	            	+
        	Test:       	TestNoForkAndExistingDir
    helpers_test.go:1143: 
        	Error Trace:	helpers_test.go:1143
        	            				helpers_test.go:385
        	Error:      	Not equal: 
        	            	expected: &gits.GitRepository{ID:0, Name:"roadrunner", AllowMergeCommit:false, HTMLURL:"https://fake.git/acme/roadrunner", CloneURL:"file:///tmp/660574327/acme", SSHURL:"", Language:"", Fork:false, Stars:0, URL:"https://fake.git/acme/roadrunner.git", Scheme:"https", Host:"fake.git", Organisation:"acme", Project:"", Private:false, HasIssues:false, OpenIssueCount:0, HasWiki:false, HasProjects:false, Archived:false}
        	            	actual  : (*gits.GitRepository)(nil)
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,23 +1,2 @@
        	            	-(*gits.GitRepository)({
        	            	- ID: (int64) 0,
        	            	- Name: (string) (len=10) "roadrunner",
        	            	- AllowMergeCommit: (bool) false,
        	            	- HTMLURL: (string) (len=32) "https://fake.git/acme/roadrunner",
        	            	- CloneURL: (string) (len=26) "file:///tmp/660574327/acme",
        	            	- SSHURL: (string) "",
        	            	- Language: (string) "",
        	            	- Fork: (bool) false,
        	            	- Stars: (int) 0,
        	            	- URL: (string) (len=36) "https://fake.git/acme/roadrunner.git",
        	            	- Scheme: (string) (len=5) "https",
        	            	- Host: (string) (len=8) "fake.git",
        	            	- Organisation: (string) (len=4) "acme",
        	            	- Project: (string) "",
        	            	- Private: (bool) false,
        	            	- HasIssues: (bool) false,
        	            	- OpenIssueCount: (int) 0,
        	            	- HasWiki: (bool) false,
        	            	- HasProjects: (bool) false,
        	            	- Archived: (bool) false
        	            	-})
        	            	+(*gits.GitRepository)(<nil>)
        	            	 
        	Test:       	TestNoForkAndExistingDir
    helpers_test.go:1149: 
        	Error Trace:	helpers_test.go:1149
        	            				helpers_test.go:385
        	Error:      	"[]" should have 1 item(s), but has 0
        	Test:       	TestNoForkAndExistingDir
FAIL

rawlingsj · 2020-07-24T11:16:16Z

The boot and terraform BDD tests are failing to deploy a quickstart into the staging environment, the staging master pipeline has the following error:

error: error getting Requirements from TeamSettings to determine if in GitHub app mode: validation failures in requirements from team settings:
velero: Additional property bucketName is not allowed
velero: Additional property resourceGroup is not allowed

rawlingsj · 2020-07-24T11:30:08Z

@chrismellard looking at the PR is it fair to say it provides a way to create buckets in azure storage? I wonder if this should be done as part of Terraform instead.

There has been an effort of work this year to decouple and move the creation + management of cloud resources out of the jx CLI and into Terraform. I wonder if that would be a better approach? What would be the main challenges doing that?

In the future we'd like to support other means of creating clusters too, https://crossplane.io/ looks really interesting as well but the more we can decouple cloud provider related commands from core jx the easier things like that will be.

WDYT?

chrismellard · 2020-07-24T11:34:18Z

Unit test appears to have been failing for a while and I'm fixing up in another PR, seems to be related to git error messages based on empty commits for rebases vs cherrypicks... though surprised given this PR contains that fix this is still failing unit tests (it certainly rectifies a unit test failure locally)

Happy to move these cloud resource creations in to terraform. Was unaware of the initiative to move this logic in to terraform ;) I'll have a think and revise this PR (+ terraform module)

rawlingsj · 2020-07-24T11:41:54Z

Ok sounds good, we need to do a better job of communicating these initiatives, I'll add an agenda item for the office hours next week to discuss ways we need to improve.

chrismellard · 2020-07-24T11:43:31Z

Unit test PR here btw - #7468

chrismellard · 2020-07-24T12:06:03Z

Having thought about this and with a view to not break other public cloud implementations how about we create a structure of cloud capabilities per cloud cluster, i.e. what we are or are not leveraging from terraform modules. Can start to data drive the code from this rather than hard coding != GKE everywhere. For this PR this will initially just contain logic informing the verify step to not create the Azure storage URLs defined in jx-requirements but just to verify their existence. We'll have a structure somewhere reflecting the fact that AKS has its storage provisioned via terraform. Omissions from this structure for any public cloud provider will imply lack of terraform capability in that area.

Does that sound like an approach everyone is happy with? Second question - are we happy to hard code this terraform capability structure in code - doesn't seem to be any benefit to me to externalise this logic.

abayer · 2020-07-28T15:06:15Z

/retest

…n operational Encapsulated the storage account parsing down close the CLI which appears to be the only part that requires them as separate arguments ... to get Velero boot operational Signed-off-by: Chris Mellard <[email protected]>

jenkins-x-bot · 2020-07-29T10:37:09Z

@chrismellard: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
boot-vault	`514f755`	link	`/test boot-vault`
tf-boot	`514f755`	link	`/test tf-boot`

View all Builds for this Pull Request

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository. I understand the commands that are listed here.

chrismellard · 2020-07-29T10:38:35Z

Will reopen another PR for this when it's nearer completion

jenkins-x-bot added the needs-ok-to-test label Jul 18, 2020

jenkins-x-bot added the size/XS label Jul 18, 2020

chrismellard force-pushed the 7450-Velero-Azure-Config branch from af67b8e to 28c81d9 Compare July 18, 2020 11:04

jenkins-x-bot added size/L and removed size/XS labels Jul 18, 2020

houndci-bot reviewed Jul 18, 2020

View reviewed changes

chrismellard force-pushed the 7450-Velero-Azure-Config branch from 62bc947 to 15e49ee Compare July 18, 2020 11:45

houndci-bot reviewed Jul 18, 2020

View reviewed changes

pkg/cloud/aks/aks.go Show resolved Hide resolved

pkg/cloud/aks/aks.go Outdated Show resolved Hide resolved

houndci-bot reviewed Jul 18, 2020

View reviewed changes

pkg/cloud/aks/aks.go Show resolved Hide resolved

pkg/cloud/aks/aks.go Outdated Show resolved Hide resolved

chrismellard force-pushed the 7450-Velero-Azure-Config branch 3 times, most recently from 00ba3f0 to dfe1cf9 Compare July 18, 2020 11:53

chrismellard changed the title ~~fix: Adding in resourceGroup and bucketName to get Velero Azure plugin operational~~ fix: Get Velero Azure plugin operational Jul 18, 2020

ankitm123 requested changes Jul 18, 2020

View reviewed changes

pkg/cloud/aks/storage/bucket_provider.go Outdated Show resolved Hide resolved

pkg/cloud/aks/aks.go Outdated Show resolved Hide resolved

jenkins-x-bot assigned ankitm123 Jul 18, 2020

chrismellard force-pushed the 7450-Velero-Azure-Config branch 2 times, most recently from 2f9d4ff to 7bfeb92 Compare July 19, 2020 09:55

chrismellard mentioned this pull request Jul 19, 2020

AKZ AZBLOB Issues in jx-requirements.yml EnsureBucketIsCreated not implemented for LegacyBucketProvider #6837

Closed

jenkins-x-bot added ok-to-test and removed needs-ok-to-test labels Jul 19, 2020

ankitm123 reviewed Jul 19, 2020

View reviewed changes

pkg/cloud/aks/storage/bucket_provider.go Outdated Show resolved Hide resolved

ankitm123 reviewed Jul 19, 2020

View reviewed changes

pkg/cloud/aks/storage/bucket_provider_test.go Show resolved Hide resolved

ankitm123 reviewed Jul 19, 2020

View reviewed changes

pkg/cloud/aks/mocks/azurestorage.go Show resolved Hide resolved

jenkins-x-bot added the size/XL label Jul 21, 2020

chrismellard force-pushed the 7450-Velero-Azure-Config branch from 2584f07 to 253461f Compare July 21, 2020 09:11

jenkins-x-bot added size/L and removed size/XL labels Jul 21, 2020

chrismellard force-pushed the 7450-Velero-Azure-Config branch from 253461f to 64f80b0 Compare July 22, 2020 00:36

chrismellard force-pushed the 7450-Velero-Azure-Config branch 2 times, most recently from 35b6b2f to 9b3d0f7 Compare July 24, 2020 09:05

houndci-bot reviewed Jul 24, 2020

View reviewed changes

pkg/gits/errors.go Outdated Show resolved Hide resolved

chrismellard force-pushed the 7450-Velero-Azure-Config branch 3 times, most recently from 65853ed to d76fa07 Compare July 24, 2020 11:12

chrismellard force-pushed the 7450-Velero-Azure-Config branch from d76fa07 to 514f755 Compare July 29, 2020 09:43

chrismellard changed the title ~~fix: Get Velero Azure plugin operational~~ wip: fix: Get Velero Azure plugin operational Jul 29, 2020

jenkins-x-bot added the do-not-merge/work-in-progress label Jul 29, 2020

chrismellard closed this Jul 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wip: fix: Get Velero Azure plugin operational #7451

wip: fix: Get Velero Azure plugin operational #7451

chrismellard commented Jul 18, 2020 •

edited

Loading

jenkins-x-bot commented Jul 18, 2020

jenkins-x-bot commented Jul 18, 2020

chrismellard commented Jul 18, 2020

ankitm123 commented Jul 18, 2020 •

edited

Loading

ankitm123 commented Jul 19, 2020

chrismellard commented Jul 19, 2020

ankitm123 commented Jul 19, 2020

rawlingsj commented Jul 21, 2020

rawlingsj commented Jul 21, 2020

ankitm123 commented Jul 22, 2020 •

edited

Loading

chrismellard commented Jul 22, 2020

rawlingsj commented Jul 24, 2020

rawlingsj commented Jul 24, 2020

rawlingsj commented Jul 24, 2020 •

edited

Loading

chrismellard commented Jul 24, 2020

rawlingsj commented Jul 24, 2020

chrismellard commented Jul 24, 2020

chrismellard commented Jul 24, 2020 •

edited

Loading

abayer commented Jul 28, 2020

jenkins-x-bot commented Jul 29, 2020

chrismellard commented Jul 29, 2020

wip: fix: Get Velero Azure plugin operational #7451

wip: fix: Get Velero Azure plugin operational #7451

Conversation

chrismellard commented Jul 18, 2020 • edited Loading

Submitter checklist

Description

Special notes for the reviewer(s)

Which issue this PR fixes

jenkins-x-bot commented Jul 18, 2020

jenkins-x-bot commented Jul 18, 2020

chrismellard commented Jul 18, 2020

ankitm123 commented Jul 18, 2020 • edited Loading

ankitm123 commented Jul 19, 2020

chrismellard commented Jul 19, 2020

ankitm123 commented Jul 19, 2020

rawlingsj commented Jul 21, 2020

rawlingsj commented Jul 21, 2020

ankitm123 commented Jul 22, 2020 • edited Loading

chrismellard commented Jul 22, 2020

rawlingsj commented Jul 24, 2020

rawlingsj commented Jul 24, 2020

rawlingsj commented Jul 24, 2020 • edited Loading

chrismellard commented Jul 24, 2020

rawlingsj commented Jul 24, 2020

chrismellard commented Jul 24, 2020

chrismellard commented Jul 24, 2020 • edited Loading

abayer commented Jul 28, 2020

jenkins-x-bot commented Jul 29, 2020

chrismellard commented Jul 29, 2020

chrismellard commented Jul 18, 2020 •

edited

Loading

ankitm123 commented Jul 18, 2020 •

edited

Loading

ankitm123 commented Jul 22, 2020 •

edited

Loading

rawlingsj commented Jul 24, 2020 •

edited

Loading

chrismellard commented Jul 24, 2020 •

edited

Loading