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

🏃 Add some events and refactor related to instance #862

Merged
merged 1 commit into from
May 15, 2021
Merged

🏃 Add some events and refactor related to instance #862

merged 1 commit into from
May 15, 2021

Conversation

hidekazuna
Copy link
Contributor

@hidekazuna hidekazuna commented May 7, 2021 •
edited

What this PR does / why we need it:

Add events

  • Delete attachinterface, trunk, and port
  • Create port and trunk
  • Replace all attributestags

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • if necessary:
    • includes documentation
    • adds unit tests

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 7, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hidekazuna

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

The pull request process is described here

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 7, 2021
chrischdi
chrischdi reviewed May 10, 2021
View reviewed changes
Copy link
Member

@chrischdi chrischdi left a comment

Choose a reason for hiding this comment

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

Hey,

I did take a look on the PR, I hope this helps :-)

pkg/cloud/services/compute/instance.go Outdated Show resolved Hide resolved
pkg/cloud/services/compute/instance.go Outdated
return trunk, nil
}

func replaceAllAttributestag(is *Service, object runtime.Object, trunkID string, tags []string) error {
Copy link
Member

Choose a reason for hiding this comment

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

Is this func is specific to trunks? If so I think we should add some context to the function name, otherwise we could rename trunkID to resourceID like it is named in the ReplaceAll func

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to express the context by function naming replaceAllAttributestag and argument name trunkID.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if this would be a good pattern. If there later is a code review and someone does a replaceAllAttributestag(is, object, port.ID, i.Tags) the context would be gone.

Also:
If we keep the name, it should be called AttributesTags (upper-case T, and s), just the package name is all lowercase and inside the gophercloud package it is also called AttributesTags

Copy link
Contributor Author

@hidekazuna hidekazuna May 12, 2021 •
edited

Choose a reason for hiding this comment

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

@chrischdi So you want to rename to replaceAllAttributesTagsOfTrunk or something?
I agree with upper-case T. This function name should start with small letter, since this function is not public.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer something like that, however I would be also ok with replaceAllAttributesTag if this is still your favourite 😌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed replaceAllAttributestag to replaceAllAttributesTags. Sorry forgot to say ending with small s.

pkg/cloud/services/compute/instance.go Show resolved Hide resolved
pkg/cloud/services/compute/instance.go Show resolved Hide resolved
pkg/cloud/services/compute/instance.go Show resolved Hide resolved
@sbueringer
Copy link
Member

@chrischdi please ping me when you're happy, then I'll also take a look :)

@chrischdi
Copy link
Member

/lgtm

@sbueringer ping

@k8s-ci-robot
Copy link
Contributor

@chrischdi: changing LGTM is restricted to collaborators

In response to this:

/lgtm

@sbueringer ping

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 kubernetes/test-infra repository.

@hidekazuna hidekazuna mentioned this pull request May 14, 2021
Merged
3 tasks
@sbueringer sbueringer mentioned this pull request May 14, 2021
Closed
@sbueringer
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 May 15, 2021
@sbueringer
Copy link
Member

@hidekazuna Imho you can squash + merge now

@hidekazuna
Add some events and refactor related to instance
8bdc74e 
Add events:
- Delete attachinterface, trunk, and port
- Create port and trunk
- Replace all attributestags
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2021
@hidekazuna
Copy link
Contributor Author

hidekazuna commented May 15, 2021 •
edited

@hidekazuna Imho you can squash + merge now

@sbueringer Thanks, Let's squash and merge. Please lgtm again.

@hidekazuna
Copy link
Contributor Author

/lgtm
/hold cancel

@k8s-ci-robot
Copy link
Contributor

@hidekazuna: you cannot LGTM your own PR.

In response to this:

/lgtm
/hold cancel

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 kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 15, 2021
@sbueringer
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 May 15, 2021
@k8s-ci-robot k8s-ci-robot merged commit b132628 into kubernetes-sigs:master May 15, 2021
@hidekazuna hidekazuna deleted the refactor_instance branch May 18, 2021 08:23
@chrischdi chrischdi mentioned this pull request May 18, 2021
Closed
6 tasks
@macaptain macaptain mentioned this pull request May 21, 2021
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrischdi chrischdi chrischdi left review comments

@sbueringer sbueringer sbueringer left review comments

@jichenjc jichenjc Awaiting requested review from jichenjc

Assignees

@sbueringer sbueringer

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.

None yet
4 participants
@hidekazuna @k8s-ci-robot @sbueringer @chrischdi