Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

refactor: swap satori's uuid package to gofrs #466

Merged
merged 1 commit into from
Feb 13, 2019

Conversation

mdanzinger
Copy link
Contributor

@mdanzinger mdanzinger commented Feb 8, 2019

Reason for Change:
Satori's go.uuid is effectively unmaintained and contained at least one critical security flaw which took half a year to be merged as the owner isn't active. See satori/go.uuid#75

Issue Fixed:

Requirements:

Notes:

@msftclas
Copy link

msftclas commented Feb 8, 2019

CLA assistant check
All CLA requirements met.

@mdanzinger
Copy link
Contributor Author

/assign @mboersma

@tariq1890
Copy link
Contributor

This is great!! Thank you @mdanzinger

@mboersma
Copy link
Member

mboersma commented Feb 8, 2019

Some import errors were caught by make test-style.

@tariq1890
Copy link
Contributor

@ultimateboy Are you okay with this Matt? IMO we should take this change as soon as possible. If you vendor aks-engine after this is merged, similar changes will have to be made in the projects that dep on aks-engine.

@tariq1890
Copy link
Contributor

@amanohar @sozercan FYI. This should go into openshift as well.

@mdanzinger
Copy link
Contributor Author

@mboersma The only new errors I receive are :

cmd/root.go:17: File is not `gofmt`-ed with `-s` (gofmt) "github.com/pkg/errors" cmd/deploy_test.go:15: File is not `gofmt`-ed with `-s` (gofmt) "github.com/pkg/errors" pkg/api/vlabs/validate.go:20: File is not `gofmt`-ed with `-s` (gofmt) "github.com/pkg/errors"

Is that what you're referring to?

Gopkg.lock Outdated Show resolved Hide resolved
@acs-bot acs-bot added size/XL and removed size/XXL labels Feb 8, 2019
@codecov
Copy link

codecov bot commented Feb 8, 2019

Codecov Report

Merging #466 into master will not change coverage.
The diff coverage is 40%.

@@           Coverage Diff           @@
##           master     #466   +/-   ##
=======================================
  Coverage   53.43%   53.43%           
=======================================
  Files          95       95           
  Lines       14391    14391           
=======================================
  Hits         7690     7690           
  Misses       6038     6038           
  Partials      663      663

@tariq1890
Copy link
Contributor

@mdanzinger The codecov patch seems to fail. Could you rebase this PR?

@acs-bot acs-bot added size/XXL and removed size/XL labels Feb 10, 2019
run gofmt

re-run dep ensure using dep 0.5.0
@mdanzinger
Copy link
Contributor Author

@tariq1890 I've rebased, squashed my previous commits, and pushed (I had to force push due to me accidentally merging two diverging branches), but it seems like the codecov patch is still failing, any ideas why?

@tariq1890
Copy link
Contributor

@mdanzinger The codecov patch diff is failing because you have changed a line which has had no coverage previously. Normally, we'd request the contributor to add tests to fill that gap. In this case, adding test cases for the said method is less straightforward and would involve too much work which would be well out of the bounds of this PR's scope.

So this PR LGTM. I would defer to the other maintainers on assessing the impact of this dep change as some projects vendor in aks-engine as a dependency.

@tariq1890
Copy link
Contributor

@jackfrancis @mboersma I have tested vendoring this fork against other repos that utilize aks-engine and I've found that it works with no issues.

@mboersma
Copy link
Member

/lgtm

@acs-bot
Copy link

acs-bot commented Feb 13, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mboersma, mdanzinger

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

@mboersma mboersma merged commit 0a6c592 into Azure:master Feb 13, 2019
@welcome
Copy link

welcome bot commented Feb 13, 2019

Congrats on merging your first pull request! 🎉🎉🎉

@tariq1890
Copy link
Contributor

Thanks @mdanzinger :)

juhacket pushed a commit to juhacket/aks-engine that referenced this pull request Mar 14, 2019
run gofmt

re-run dep ensure using dep 0.5.0
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants