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

Move resource collection logic from migration controller #344

Merged
merged 4 commits into from
May 3, 2019

Conversation

disrani-px
Copy link
Contributor

Can be used by other modules to get unstructured objects from a namepsace
and matching label selectors

What type of PR is this?
Cleanup

What this PR does / why we need it:
Refactored code so that same code can be shared by multiple modules

Does this PR change a user-facing CRD or CLI?:
No

Is a release note needed?:
No

Does this change need to be cherry-picked to a release branch?:
No

@disrani-px disrani-px added the cleanup The change performs cleanup on existing code label Apr 16, 2019
@disrani-px disrani-px added this to the 2.3.0 milestone Apr 16, 2019
return object, nil
}

func (r *ResourceCollector) prepareServiceResource(

Choose a reason for hiding this comment

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

Add a comment to all the perpareXXXResource functions, as to what they are doing and why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

prepareServiceResource does exactly one thing and there is a comment inline

@disrani-px disrani-px force-pushed the resource_collector branch 5 times, most recently from d1c10c5 to 303d46e Compare April 22, 2019 23:06
}

switch resource.Kind {
case "PersistentVolumeClaim",
Copy link

@harsh-px harsh-px Apr 23, 2019

Choose a reason for hiding this comment

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

we should allow users to configure this list as with CRDs, we would skip those leading to a partial migration. If this is configurable, users can feed in their new types without a stork update.

Also I think we discussed this before but this needs to have storage classes. A statefulset scale up will fail post migration, which means we aren't doing a complete statefulset migration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already an open issue for migrating CRs and CRDs (#282). There is more effort required to support that than just allowing them here since the resources created by the CRs don't need to be migrated themselves. Also, need to be able to handle cases where the CRD isn't registered on the destination.

Storageclasses are cluster specific, admins will create them during cluster install. Although a good to have I don't see that as a blocker for anything.

@disrani-px disrani-px force-pushed the resource_collector branch 3 times, most recently from a37ca2f to 9ddc334 Compare May 2, 2019 00:32
* Can be used by other modules to get unstructured objects from a namepsace
and matching label selectors
* Added support to collect clusterrolebindings inlcuding users and groups for a
namespace
* Added merging of clusterrolebindings when applying resources
@disrani-px disrani-px force-pushed the resource_collector branch from dbbe3a9 to f66e649 Compare May 2, 2019 00:40
@codecov
Copy link

codecov bot commented May 2, 2019

Codecov Report

Merging #344 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #344   +/-   ##
=======================================
  Coverage   72.82%   72.82%           
=======================================
  Files          24       24           
  Lines        2112     2112           
=======================================
  Hits         1538     1538           
  Misses        444      444           
  Partials      130      130
Impacted Files Coverage Δ
pkg/monitor/monitor.go 46.22% <0%> (ø) ⬆️

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 79c82f8...dc84872. Read the comment docs.

@disrani-px disrani-px merged commit 286c9de into master May 3, 2019
@disrani-px disrani-px deleted the resource_collector branch May 14, 2019 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup The change performs cleanup on existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants