Skip to content

Conversation

@astaticvoid
Copy link
Contributor

@astaticvoid astaticvoid commented Oct 28, 2021

Issue #, if available: 45

Description of changes:
Create janitor script to clean up all resources under a namespace in Cloud Map, to reset test account in between integration test executions.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2021

Codecov Report

Merging #57 (7a85132) into main (075bf73) will increase coverage by 2.66%.
The diff coverage is 80.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #57      +/-   ##
==========================================
+ Coverage   47.70%   50.36%   +2.66%     
==========================================
  Files           9       11       +2     
  Lines        1000     1082      +82     
==========================================
+ Hits          477      545      +68     
- Misses        502      512      +10     
- Partials       21       25       +4     
Impacted Files Coverage Δ
integration/janitor/main.go 0.00% <0.00%> (ø)
pkg/cloudmap/api.go 62.65% <0.00%> (+0.37%) ⬆️
pkg/cloudmap/aws_facade.go 100.00% <ø> (ø)
integration/janitor/janitor.go 84.41% <84.41%> (ø)
pkg/cloudmap/client.go 54.31% <100.00%> (+0.59%) ⬆️

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 075bf73...7a85132. Read the comment docs.

return "", nil
}

func (j *janitor) deregInsts(ctx context.Context, svcId string) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's just avoid too much of shortening of the function names.

Copy link
Contributor Author

@astaticvoid astaticvoid Oct 29, 2021

Choose a reason for hiding this comment

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

Unreadable clunky abbreviations are idiomatic in go/Unix culture (kubectl? gobcon?), especially for anything we’re not exporting but we can be more verbose like Java.

@astaticvoid astaticvoid linked an issue Oct 28, 2021 that may be closed by this pull request
vanekjar
vanekjar previously approved these changes Oct 29, 2021
runakash
runakash previously approved these changes Oct 29, 2021
@astaticvoid astaticvoid dismissed stale reviews from runakash and vanekjar via 6af6269 October 29, 2021 20:22
Make ListInstances return instance summaries for reusability
@astaticvoid
Copy link
Contributor Author

astaticvoid commented Oct 29, 2021

Refactored ListInstances in API class to return list of instance summaries, in favour of deferring endpoint conversion in the client. This allows us to re-use the call for janitor.
Note that we are returning a list of structs and not pointers, as the summaries from the SDK are now collected without conversion, and we have to be careful not to return a collection of duplicate pointers. Modified a test to ensure ListInstances can return multiple results to catch this. Probably our other list functions should do the same.

Copy link
Member

@runakash runakash left a comment

Choose a reason for hiding this comment

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

@astaticvoid astaticvoid merged commit a132823 into aws:main Nov 1, 2021
@astaticvoid astaticvoid deleted the dhemmerl-janitor branch November 1, 2021 17:27
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.

Implement end-to-end test suite

4 participants