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

Deployment check tol fix #783

Closed
wants to merge 18 commits into from
Closed

Deployment check tol fix #783

wants to merge 18 commits into from

Conversation

jdowni000
Copy link
Contributor

Deployment check fix that now has the ability to accept multiple tolerations. Additionally, it now checks for the toleration to be null and does not create any tolerations. I have tested this in lab and has accepted 3 tolerations, single tolerations, and no tolerations. All tests passed and scheduled as expected.

cmd/deployment-check/Makefile Outdated Show resolved Hide resolved
cmd/deployment-check/README.md Outdated Show resolved Hide resolved
cmd/deployment-check/main.go Outdated Show resolved Hide resolved
go.sum Outdated
@@ -14,15 +14,20 @@ cloud.google.com/go/pubsub v1.0.1/go.mod h1:R0Gpsv3s54REJCy4fxDixWD93lHJMoZTyQ2k
cloud.google.com/go/storage v1.0.0/go.mod h1:IhtSnM/ZTZV8YYJWCY8RULGVqBDmpoyjwiyrjsg+URw=
dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=
github.com/Azure/go-autorest/autorest v0.9.0/go.mod h1:xyHB1BMZT0cuDHU7I0+g046+BFDTQ8rEZB0s4Yfa6bI=
github.com/Azure/go-autorest/autorest v0.9.6 h1:5YWtOnckcudzIw8lPPBcWOnmIFWMtHci1ZWAZulMSx0=
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are there so many Azure packages added?

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 do not have answer for this. I do not believe I imported any new packages for this check revision

Comment on lines 304 to 307
if splitEnvVars == nil {
err := errors.New("nil was entered into multiple toleration parser")
ReportFailureAndExit(err)
}
Copy link
Collaborator

@joshulyne joshulyne Jan 22, 2021

Choose a reason for hiding this comment

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

you do a check in the tolerationHandler func that makes sure the list has more than 1 item:
if len(splitEnvVars) > 1 { before you pass it in the multipleTolerations func.
so is this check if splitEnvVars == nil be necessary?

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 received feedback to always have something like this in to make sure that no one is abusing the function. I understand it is repetitive. Let me know how you want me to proceed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thats fair, i wasn't thinking that it was a function that could be reused! 👍

cmd/deployment-check/README.md Outdated Show resolved Hide resolved
cmd/deployment-check/README.md Outdated Show resolved Hide resolved
cmd/deployment-check/deployment-check.yaml Outdated Show resolved Hide resolved
@integrii integrii added this to the 2.5.0 milestone Jan 22, 2021
@jonnydawg
Copy link
Collaborator

I am working on an edit right now

@jonnydawg jonnydawg mentioned this pull request Jan 25, 2021
@jdowni000 jdowni000 closed this Jan 27, 2021
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.

None yet

4 participants