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.

Sign up for GitHub

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

Jump to bottom

Refactor toolbox dump & dump structured instances #3719

Merged
merged 1 commit into from
Oct 29, 2017
Merged

Refactor toolbox dump & dump structured instances #3719

merged 1 commit into from
Oct 29, 2017

Conversation

justinsb
Copy link
Member

This will enable log collection even if nodes don't register.

AWS: Dumps ids & addresses
GCE: Dumps names - addresses to follow
Others: Not yet!

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 28, 2017
@justinsb
Refactor toolbox dump & dump structured instances
a18363f 
This will enable log collection even if nodes don't register.

AWS: Dumps ids & addresses
GCE: Dumps names - addresses to follow
Others: Not yet!
@justinsb justinsb force-pushed the tooldump_dump_instances branch from 4d50513 to a18363f Compare October 28, 2017 18:46
@justinsb justinsb requested a review from andrewsykim October 28, 2017 19:04
@justinsb
Copy link
Member Author

Hi @andrewsykim want to take a look at this one? I think you previously split out the tracker package to make it easier to implement DO, but I think this may be an easier way...

The idea is that we have the base package pkg/resources, and the core objects live in there. We will have the per-cloud implementations pkg/resources/do /pkg/resources/aws etc, which depend on pkg/resources. (I haven't moved AWS or GCE ... yet ;-) ). And then this was previously a circular dependency, but what I did was created a utils pkg/resources/utils which depends on basically all the packages, but nobody depends on it, and that is where the dispatch function goes. I don't love the utils name - suggestions welcome :-)

@justinsb
Copy link
Member Author

Also @andrewsykim not urgent, and let me know if you want me to break it up into the refactor and the dump-instances bit - I should have done that, but the dump-instances bit is sufficiently simple that you might let me get away with it ;-)

andrewsykim
andrewsykim reviewed Oct 29, 2017
View reviewed changes
cmd/kops/delete_cluster.go
@@ -180,7 +176,7 @@ func RunDeleteCluster(f *util.Factory, out io.Writer, options *DeleteClusterOpti

fmt.Fprintf(out, "\n")

err = d.DeleteResources(clusterResources)
err = resources.DeleteResources(cloud, clusterResources)
Copy link
Member

Choose a reason for hiding this comment

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

Can we put DeleteResources into resource/utils to be consistent with ListResources? Or does that create a circular dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

We definitely can :-) Did that as a separate additional commit in #3720 to (hopefully) make it easier to review.

This does suggest that maybe this should be pkg/resource/operations instead of pkg/resources/utils - maybe?

This was referenced Oct 29, 2017
Merged
Merged
@andrewsykim
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 29, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue.

@k8s-github-robot k8s-github-robot merged commit dd207dc into kubernetes:master Oct 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewsykim andrewsykim andrewsykim left review comments

Assignees

@andrewsykim andrewsykim

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@justinsb @andrewsykim @k8s-github-robot @k8s-ci-robot