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

Cleanup some TODOs #218

Merged
merged 3 commits into from
Jun 27, 2019
Merged

Cleanup some TODOs #218

merged 3 commits into from
Jun 27, 2019

Conversation

ahmetb
Copy link
Member

@ahmetb ahmetb commented Jun 19, 2019

- cleaned up some obsolete todos (ref: kubernetes-sigs#213), some still relevant
- removed PluginIndex type, as it wasn't used for any reason

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 19, 2019
@ahmetb
Copy link
Member Author

ahmetb commented Jun 19, 2019

/assign @corneliusweig

@k8s-ci-robot
Copy link
Contributor

@ahmetb: GitHub didn't allow me to assign the following users: corneliusweig.

Note that only kubernetes-sigs members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @corneliusweig

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 added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 19, 2019
@codecov-io
Copy link

codecov-io commented Jun 19, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@4b0255c). Click here to learn what that means.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #218   +/-   ##
=========================================
  Coverage          ?   53.78%           
=========================================
  Files             ?       14           
  Lines             ?      701           
  Branches          ?        0           
=========================================
  Hits              ?      377           
  Misses            ?      273           
  Partials          ?       51
Impacted Files Coverage Δ
pkg/index/validate.go 94.44% <ø> (ø)
cmd/validate-krew-manifest/main.go 58.33% <0%> (ø)
pkg/index/indexscanner/scanner.go 67.34% <71.42%> (ø)

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 4b0255c...6009809. Read the comment docs.

Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Just one nit and and a few wording suggestions. Feel free to reject whatever you dislike :)

pkg/index/indexscanner/scanner.go Outdated Show resolved Hide resolved
pkg/index/indexscanner/scanner.go Outdated Show resolved Hide resolved
pkg/index/types.go Outdated Show resolved Hide resolved
@@ -38,7 +37,8 @@ type PluginSpec struct {
Platforms []Platform `json:"platforms,omitempty"`
}

// Platform TODO(lbb)
// Platform describes the how to match to a particular platform (os, arch) and
Copy link
Contributor

Choose a reason for hiding this comment

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

I find that comment easier to read, if it is reversed, like so:

// Platform describes how to perform an installation on a specific platform
// and how to match the target platform (os, arch).

pkg/index/types.go Outdated Show resolved Hide resolved
@corneliusweig
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, corneliusweig

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
Copy link
Contributor

@corneliusweig: changing LGTM is restricted to assignees, and only kubernetes-sigs/krew repo collaborators may be assigned issues.

In response to this:

/lgtm
/approve

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.

@corneliusweig
Copy link
Contributor

/shrug

@k8s-ci-robot k8s-ci-robot added the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Jun 19, 2019
@corneliusweig
Copy link
Contributor

/shrug cancel

Note: trying github's code review suggestions feature. Might cause CLA check to break.

Co-Authored-By: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2019
@ahmetb
Copy link
Member Author

ahmetb commented Jun 27, 2019

Merging manually since approved manually.

@ahmetb ahmetb merged commit 083a44a into kubernetes-sigs:master Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants