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

add Windows node support #185

Merged
merged 8 commits into from
Jul 6, 2020
Merged

add Windows node support #185

merged 8 commits into from
Jul 6, 2020

Conversation

cjerad
Copy link
Contributor

@cjerad cjerad commented Jun 29, 2020

Issue #, if available:
Resolves #8

Description of changes:
Adds support for running on Windows nodes. Also updates build and test scripts.

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

@cjerad cjerad requested a review from bwagner5 June 29, 2020 20:27
@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2020

Codecov Report

Merging #185 into master will decrease coverage by 0.06%.
The diff coverage is 91.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
- Coverage   81.30%   81.24%   -0.07%     
==========================================
  Files           8       10       +2     
  Lines         765      773       +8     
==========================================
+ Hits          622      628       +6     
- Misses        128      129       +1     
- Partials       15       16       +1     
Impacted Files Coverage Δ
pkg/node/node.go 58.38% <80.00%> (-1.30%) ⬇️
pkg/config/config.go 100.00% <100.00%> (ø)
pkg/uptime/common.go 100.00% <100.00%> (ø)
pkg/uptime/uptime_linux.go 100.00% <100.00%> (ø)

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 f16d712...eaf55cd. Read the comment docs.

@cjerad cjerad marked this pull request as ready for review July 1, 2020 14:56
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.

a couple of comments on the run unit tests thru docker script to simplify.

project_root_dir="$(cd "$(dirname "$0")/.." && pwd -P)"
work_dir="/workplace/aws-node-termination-handler"
container_name="nth_unit_test_on_linux"
deps="go,git,make"
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of using alpine and installing dependencies, you can use the golang:1 docker container which already has go, git, and make installed.

IFS=',' read -ra deps <<< "$deps"
echo "dependencies to install: ${deps[@]}"

docker container create \
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the default workflow would be to run the unit tests and discard the container. Maybe change "recreate" to "preserve" instead which would effectively remove the --rm from the command below.

docker run -it --rm -e GOPROXY=direct --volume "$project_root_dir:$work_dir" --workdir "$work_dir" --name "$container_name" golang:1

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.

Overall, looks very good! Going to go ahead and merge. We'll do some cleanup and polishing in smaller PRs.

@bwagner5 bwagner5 merged commit f5f8766 into aws:master Jul 6, 2020
@cjerad cjerad deleted the issue-8 branch July 7, 2020 18:31
@yurrriq
Copy link
Contributor

yurrriq commented Aug 4, 2020

I'm getting the following error since upgrading from 0.8.0 to 0.9.2. Seems potentially related to this PR. I have not set that value.

Error: Failed to render chart: exit status 1: Error: template: aws-node-termination-handler/templates/daemonset.linux.yaml:28:85: executing "aws-node-termination-handler/templates/daemonset.linux.yaml" at <.Values.linuxPodAnnotations>: wrong type for value; expected map[string]interface {}; got interface {}

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.

Support windows spot instances
4 participants