Skip to content

don't exit(1) in queue processor mode #422

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

Merged
merged 3 commits into from
May 11, 2021

Conversation

heschlie
Copy link
Contributor

@heschlie heschlie commented Apr 30, 2021

Issue #, if available: #412

Description of changes: Don't exit(1) on drain errors when in queue processor mode

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@bwagner5 bwagner5 self-requested a review May 5, 2021 20:26
@heschlie
Copy link
Contributor Author

heschlie commented May 7, 2021

So, I'm having trouble with the run-report-card-test.sh test. It seems like there might be a couple errors in it, but I'm wondering how it ever worked and if I'm just dumb:

shouldn't this line be...

if [[ ! $(docker run -it -v $SCRIPTPATH/../..:/app go-report-card-cli /go/bin/goimports -l /app) ]]; then

As goimports should only return an error if it finds offending imports?

Also, this line should probably exclude the vendor directory? Also it seems that goreportcard-cli needs the directory specified in order to skip the vendor dir, this wasn't erroring locally but it was noisy.

I've made these changes, please let me know if I should revert them. Again I feel like if it was a problem it would have shown up before and I'm just missing something.

@brycahta
Copy link
Contributor

brycahta commented May 7, 2021

Hey @heschlie , thanks for the PR!

We have an open issue #424 to fix go-report-card as we saw it start to break recently, so no worries if it's failing on your changes. I should get the fix completed early next week

@heschlie
Copy link
Contributor Author

heschlie commented May 7, 2021

Hey @brycahta thanks for the heads up, as for the E2E tests it looks to be failing on the first E2E script:

======================================================================================================
🥑 Running assertion script asg-lifecycle-sqs-test
======================================================================================================
Starting ASG Lifecycle SQS Test for Node Termination Handler
+ helm upgrade --install nth-test-2377abf5-localstack /home/runner/work/aws-node-termination-handler/aws-node-termination-handler/test/e2e/../../config/helm/localstack/ --wait --namespace default --set 'nodeSelector.kubernetes\.io/hostname=nth-test-2377abf5-control-plane' --set defaultRegion=us-east-1
Release "nth-test-2377abf5-localstack" does not exist. Installing it now.
Error: timed out waiting for the condition

Is there a way for me to get more detail on why helm (I'm assuming) timed out?

Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

lgtm thanks!

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