Skip to content

CLI: Migration create#111

Merged
openshift-merge-robot merged 1 commit into
backube:mainfrom
vh05:migration-create
Feb 1, 2022
Merged

CLI: Migration create#111
openshift-merge-robot merged 1 commit into
backube:mainfrom
vh05:migration-create

Conversation

@vh05

@vh05 vh05 commented Nov 29, 2021

Copy link
Copy Markdown
Contributor

Describe what this PR does
This sub-command should prepare a destination to receive incoming transfers from an external source.

  • Saves relationship info
  • Creates the Namespace if it doesn't exist
  • Creates a PVC in the Namespace if it doesn't exist
  • Creates an associated ReplicationDestination

@codecov

codecov Bot commented Nov 29, 2021

Copy link
Copy Markdown

Codecov Report

Merging #111 (ee58915) into main (1d4657e) will decrease coverage by 0.7%.
The diff coverage is 58.4%.

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

@@           Coverage Diff           @@
##            main    #111     +/-   ##
=======================================
- Coverage   74.2%   73.5%   -0.8%     
=======================================
  Files         19      19             
  Lines       1715    1889    +174     
=======================================
+ Hits        1274    1389    +115     
- Misses       353     395     +42     
- Partials      88     105     +17     
Impacted Files Coverage Δ
kubectl-volsync/cmd/migration.go 55.0% <43.7%> (-45.0%) ⬇️
kubectl-volsync/cmd/migration_create.go 57.5% <59.7%> (+40.3%) ⬆️
kubectl-volsync/cmd/relationship.go 66.6% <0.0%> (+4.3%) ⬆️

@vh05 vh05 changed the title [WIP]CLI: Migration create CLI: Migration create Nov 30, 2021

@JohnStrunk JohnStrunk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think a lot of the code is there, it just needs a bit of rearrangement.

Comment thread kubectl-volsync/cmd/migration.go
Comment thread kubectl-volsync/cmd/migration.go
Comment thread kubectl-volsync/cmd/migration.go Outdated
Comment thread kubectl-volsync/cmd/migration.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration.go
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go
@openshift-ci openshift-ci Bot added size/XL and removed size/L labels Dec 16, 2021
@vh05

vh05 commented Dec 16, 2021

Copy link
Copy Markdown
Contributor Author

/retest

Comment thread kubectl-volsync/cmd/migration_create_test.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration.go
Comment thread kubectl-volsync/cmd/migration.go
@vh05
vh05 force-pushed the migration-create branch from 97b4968 to f91c38f Compare January 6, 2022 07:44
Comment thread kubectl-volsync/cmd/migration.go Outdated
Comment thread kubectl-volsync/cmd/migration.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_test_helper.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
@vh05
vh05 force-pushed the migration-create branch from 7c5e98d to 8a198d4 Compare January 14, 2022 07:40
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Comment thread kubectl-volsync/cmd/migration_create.go
Comment thread kubectl-volsync/cmd/migration_create.go Outdated
@vh05
vh05 force-pushed the migration-create branch from f91277d to 6bc4808 Compare January 20, 2022 11:09

@JohnStrunk JohnStrunk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Moving on to reviewing the tests...

It's also time to make sure CI is green. make test should pass.

Comment thread kubectl-volsync/cmd/migration_create_test.go Outdated
Comment thread kubectl-volsync/cmd/migration_create_test.go Outdated
Comment thread kubectl-volsync/cmd/migration_create_test.go Outdated
Comment thread kubectl-volsync/cmd/migration_create_test.go Outdated
Comment thread kubectl-volsync/cmd/migration_create_test.go
Comment thread kubectl-volsync/cmd/migration_create_test.go Outdated
Comment thread kubectl-volsync/cmd/migration_create_test.go Outdated
Comment thread kubectl-volsync/cmd/migration_create_test.go Outdated
@vh05

vh05 commented Jan 23, 2022

Copy link
Copy Markdown
Contributor Author

/retest

@vh05

vh05 commented Jan 23, 2022

Copy link
Copy Markdown
Contributor Author

Moving on to reviewing the tests...

It's also time to make sure CI is green. make test should pass.

I am not sure the coverage file needs to be explicitly created. Even on local run it fails with similar error

KUBEBUILDER_ASSETS="/home/runner/.local/share/kubebuilder-envtest/k8s/1.22.1-linux-amd64" /home/runner/work/volsync/volsync/bin/ginkgo -progress -randomizeAllSpecs -randomizeSuites -slowSpecThreshold 30 -p -cover -coverprofile cover.out -outputdir . ./...
[1642935250] Rsync mover - 38 specs - 2 nodes •••••••••••••••••••••••••••••••••••••• SUCCESS! 29.357647612s 
[1642935250] Controller - 89 specs - 2 nodes ••••••••••••••••••••P•••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••• SUCCESS! 1m29.907525823s 
[1642935250] Volumehandler - 8 specs - 2 nodes •••••••• SUCCESS! 12.83253519s 
[1642935250] Restic mover - 33 specs - 2 nodes ••••••••••••••••••••••••••••••••• SUCCESS! 15.776819564s 
[1642935250] kubectl-volsync Suite - 20 specs - 2 nodes ••••
	 -------------------------------------------------------------------
	|                                                                   |
	|  Ginkgo timed out waiting for all parallel nodes to report back!  |
	|                                                                   |
	 -------------------------------------------------------------------

 cmd timed out. path: ./kubectl-volsync/cmd
path is /home/runner/work/volsync/volsync
Unable to read coverage file  to combine, open : no such file or directory

Ginkgo ran 5 suites in 3m12.162999893s
Test Suite Failed

@JohnStrunk

Copy link
Copy Markdown
Member

Moving on to reviewing the tests...
It's also time to make sure CI is green. make test should pass.

I am not sure the coverage file needs to be explicitly created. Even on local run it fails with similar error

The timeout error is because the tests are broken (in a way that is more serious than just an Expect failing)--- and because of the error, coverage files aren't properly generated.

@vh05

vh05 commented Jan 24, 2022

Copy link
Copy Markdown
Contributor Author

Moving on to reviewing the tests...
It's also time to make sure CI is green. make test should pass.

I am not sure the coverage file needs to be explicitly created. Even on local run it fails with similar error

The timeout error is because the tests are broken (in a way that is more serious than just an Expect failing)--- and because of the error, coverage files aren't properly generated.

Is it possible to increase the timeout value to some bigger value?

@vh05

vh05 commented Jan 25, 2022

Copy link
Copy Markdown
Contributor Author

/retest

@vh05
vh05 force-pushed the migration-create branch from 84187b2 to 19457fb Compare January 25, 2022 05:27

@JohnStrunk JohnStrunk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Other than the nil check, I think this is ready.
Please squash and force push after fixing the nil check.

Comment thread kubectl-volsync/cmd/migration_create.go Outdated
Describe what this PR does
This sub-command should prepare a destination to
receive incoming transfers from an external source.

Creates the Namespace if it doesn't exist
Creates a PVC in the Namespace if it doesn't exist
Creates an associated ReplicationDestination
Saves relationship info

Signed-off-by: Vinayakswami Hariharmath <vharihar@redhat.com>
@vh05
vh05 force-pushed the migration-create branch from ee58915 to bacda63 Compare February 1, 2022 04:03
@JohnStrunk

Copy link
Copy Markdown
Member

/lgtm

@openshift-ci openshift-ci Bot added the lgtm label Feb 1, 2022
@JohnStrunk

Copy link
Copy Markdown
Member

/approve

@openshift-ci

openshift-ci Bot commented Feb 1, 2022

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JohnStrunk, v-harihar

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

The pull request process is described here

Details 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

@openshift-ci openshift-ci Bot added the approved label Feb 1, 2022
@openshift-merge-robot
openshift-merge-robot merged commit f84d09f into backube:main Feb 1, 2022
@JohnStrunk JohnStrunk mentioned this pull request Feb 1, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants