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

make matching a strict subset of labels #190

Closed
wants to merge 1 commit into from
Closed

make matching a strict subset of labels #190

wants to merge 1 commit into from

Conversation

eiais
Copy link
Contributor

@eiais eiais commented Nov 23, 2017

Signed-off-by: Kyle Spiers kyle@spiers.me

@eiais
Copy link
Contributor Author

eiais commented Nov 24, 2017

closes #158

@kisom
Copy link
Contributor

kisom commented Nov 27, 2017

@eiais it looks like this caused the build to fail.

@eiais
Copy link
Contributor Author

eiais commented Nov 29, 2017

@kisom Yeah, there's a test in core_test that needs fixing. Also this could be a breaking change for some users I think. Is that something you would be concerned with merging?

Copy link
Contributor

@kisom kisom left a comment

Choose a reason for hiding this comment

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

The code is fine, and I'd be fine with merging it, but it also needs go fmt run over it.

for _, label := range labels {
if label == validLabel {
return true
//Given an asset usage label must be a superset
Copy link
Contributor

Choose a reason for hiding this comment

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

Though this code predates some of our best practices (and the comment does match the style of other comments), it should follow standard Go practices now; e.g.

// Given an asset, usage label must be a superset.

Signed-off-by: Kyle Spiers <kyle@spiers.me>
@codecov-io
Copy link

Codecov Report

Merging #190 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #190      +/-   ##
==========================================
+ Coverage   35.57%   35.62%   +0.05%     
==========================================
  Files          21       21              
  Lines        4964     4968       +4     
==========================================
+ Hits         1766     1770       +4     
  Misses       2963     2963              
  Partials      235      235
Impacted Files Coverage Δ
keycache/keycache.go 50.57% <100%> (+0.76%) ⬆️

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 746a508...25a9b17. Read the comment docs.

@eiais
Copy link
Contributor Author

eiais commented Nov 29, 2017

@kisom Should be good to go now.

@kisom
Copy link
Contributor

kisom commented Nov 29, 2017

@eiais I'm going to have to do an audit of some our usage of RO this week to figure out if this will negatively impact anything, I'll keep you updated.

@kisom kisom self-assigned this Nov 29, 2017
@eiais
Copy link
Contributor Author

eiais commented Dec 14, 2017

@kisom Any update?

@eiais
Copy link
Contributor Author

eiais commented Feb 22, 2018

Should I close this PR?

@eiais
Copy link
Contributor Author

eiais commented Apr 12, 2018

@grittygrease Can I get an update on this?

@eiais eiais closed this May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants