fix(runners): Namespace `Application` tag #2182

mcaulifn · 2022-06-22T21:48:52Z

Namespaces the Application by ghr:Application tag to avoid conflicting with Application tag set by policies.

Resolves #2143

mcaulifn · 2022-06-22T21:58:17Z

@npalm Seems the TF checks are broken?

npalm · 2022-06-23T14:49:33Z

For some reason the CI is failing, can you double check you are in sync? Should not fail since no TF files are changed.

mcaulifn · 2022-06-23T17:43:13Z

Thought I had the latest. Pulled down develop again but still failing.

npalm · 2022-06-23T20:55:41Z

Strange, since develop and main are building without an issue. Including todays release.

mcaulifn · 2022-06-24T17:25:01Z

@npalm Looks like one of the policies had some unintended formatting applied. I revered and all checks are passing.

mcaulifn · 2022-06-29T17:38:11Z

@npalm Looks like this might be a one-time break where users will have to clean out old runners. OK to keep as is or do you have suggestions?

npalm · 2022-06-30T11:35:54Z

@mcaulifn not a really good idea rigiht now. Only option coming to my mind is let the scale down lambda be aware of the old and new tag. And remove the old tag in a next major release.

npalm · 2022-07-06T10:01:04Z

@mcaulifn it the PR ready for review?

mcaulifn · 2022-07-06T14:55:32Z

@npalm Should be good now

npalm · 2022-07-13T16:28:34Z

modules/runners/lambdas/runners/src/aws/runners.ts

 export async function listEC2Runners(filters: ListRunnerFilters | undefined = undefined): Promise<RunnerList[]> {
-  const ec2Statuses = filters?.statuses ? filters.statuses : ['running', 'pending'];
-  const ec2 = new EC2();
-  const ec2Filters = [


Not sure if it is really needed, but back in the days we have implemented here hardcoded that if not set a filter. A filter got enforced. I think this was done to avoid the scale down can terminate anything that is not owned.

Don't like this hidden magic and we should got cleaned it at some moment. I also updated the tested to see the impact when we remove the depecrated tag Application. In that case you will find that the test for instance we no tags 'Instances with no tags.' failes. Which is related to these default.s

Which test did you update? I'll replicate and resolve.

The test Instances with no tags. on L134 in on this branch. Sorry for the late reply

'Instances with no tags.' should be changed to expect 0 and now it is failing for me.

Despite mockReturnValueOnce being used throughout, and that instance definition not being used in the test in question, I am getting the two instances used at the top of the test. This appears to be an issue with the test rather than with the code. I'm still trying to track down how it is getting injected.

Seems this bug is being hit. I'll try to work up a fix.

I have not been able to find a work-around for the testing bug. @npalm any suggestions?

I check the tests again.

At least I know what the problem is. When I remove the support vor the tag Application in (runner.ts). Merge the two mocks back in a single mock like before, returning 2 objects. The test "Instances with no tags." starts failing. Running the test in isolation is not causing a problem. So somehow the mocks are not resetting well.

Was not able to find a solution, but this eems the issue.

github-actions · 2022-09-04T02:16:25Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs. Thank you for your contributions.

npalm · 2022-09-16T15:03:24Z

@mcaulifn back from PTO, will try to check what I can do on this PR next week.

npalm · 2022-10-14T15:56:50Z

Will dig in the PR soon, we are working on several breaking changes. So would like to got this one merged soon. That gives the option to clean up logic together with #2517

github-actions · 2022-11-14T02:14:13Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs. Thank you for your contributions.

npalm · 2022-11-16T22:35:56Z

@mcaulifn I just started digging in the PR and running some test, keep you posted.

modules/runners/policies/lambda-scale-down.json

modules/runners/lambdas/runners/src/aws/runners.ts

npalm · 2022-11-16T23:24:20Z

@mcaulifn found 2 issues, would you have tim for a quick look?

modules/runners/lambdas/runners/src/aws/runners.test.ts

npalm

@mcaulifn PR is good with one small request, can you add the permission as suggested in the comment. Thanks!

modules/runners/policies/lambda-scale-down.json

npalm · 2022-11-24T07:43:12Z

@mcaulifn thanks will have asap another check on the PR!

npalm

@mcaulifn once again thanks for you support. I already have prepared the PR to remove the legacy tag (#2705)

## [1.17.0](philips-labs/terraform-aws-github-runner@v1.16.1...v1.17.0) (2022-11-30) ### Features * **runners:** Namespace `Application` tag ([#2182](https://github.com/philips-labs/terraform-aws-github-runner/issues/2182)) ([a1a47a4](philips-labs/terraform-aws-github-runner@a1a47a4)) ### Bug Fixes * Adding missing input lambda vpc vars to syncer module ([#2701](https://github.com/philips-labs/terraform-aws-github-runner/issues/2701)) ([c91a96b](philips-labs/terraform-aws-github-runner@c91a96b))

mcaulifn added 2 commits June 22, 2022 17:46

fix(tag): Namespace Application tag

ac26958

Update tests

2d805c8

Merge branch 'develop' into mcaulifn/app-tag

c10dc91

Reverting format

95abc52

npalm self-requested a review June 27, 2022 20:18

npalm changed the title ~~fix(tag): Namespace Application tag~~ fix(runners): Namespace Application tag Jun 27, 2022

Support legacy Application tag key

707012f

npalm added the stale:exempt label Jul 13, 2022

npalm reviewed Jul 13, 2022

View reviewed changes

github-actions bot added the Stale label Sep 4, 2022

Merge branch 'develop' into mcaulifn/app-tag

1bfafc5

github-actions bot removed the Stale label Sep 7, 2022

Merge branch 'develop' into mcaulifn/app-tag

321cd35

Merge branch 'develop' into mcaulifn/app-tag

af34741

npalm mentioned this pull request Nov 13, 2022

breaking release V2 to support multi-runners #2517

Closed

17 tasks

github-actions bot added the Stale label Nov 14, 2022

npalm removed the Stale label Nov 14, 2022

Merge branch 'develop' into mcaulifn/app-tag

66bcc58

npalm reviewed Nov 16, 2022

View reviewed changes

modules/runners/policies/lambda-scale-down.json Outdated Show resolved Hide resolved

npalm reviewed Nov 16, 2022

View reviewed changes

modules/runners/lambdas/runners/src/aws/runners.ts Show resolved Hide resolved

npalm reviewed Nov 17, 2022

View reviewed changes

modules/runners/lambdas/runners/src/aws/runners.test.ts Show resolved Hide resolved

npalm requested changes Nov 17, 2022

View reviewed changes

mcaulifn added 3 commits November 18, 2022 14:05

Adding legacy StringEquals

8baf486

Reverting test

1eb3612

Merge branch 'develop' into mcaulifn/app-tag

069d87f

npalm reviewed Nov 18, 2022

View reviewed changes

modules/runners/policies/lambda-scale-down.json Outdated Show resolved Hide resolved

Removing new lines

5e7f145

npalm approved these changes Nov 30, 2022

View reviewed changes

npalm merged commit a1a47a4 into github-aws-runners:develop Nov 30, 2022

npalm mentioned this pull request Dec 13, 2022

fix(runners): Remove Application legacy tag #2705

Merged

forest-releaser bot mentioned this pull request Dec 28, 2022

chore(release-cleanup): release 1.14.0 #2815

Closed

npalm mentioned this pull request Dec 28, 2022

chore(main): release 2.0.0 #2821

Merged

forest-releaser bot mentioned this pull request Apr 28, 2023

chore(main): release 4.0.0 #3233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(runners): Namespace `Application` tag #2182

fix(runners): Namespace `Application` tag #2182

mcaulifn commented Jun 22, 2022 •

edited by npalm

Loading

mcaulifn commented Jun 22, 2022

npalm commented Jun 23, 2022

mcaulifn commented Jun 23, 2022

npalm commented Jun 23, 2022

mcaulifn commented Jun 24, 2022

mcaulifn commented Jun 29, 2022

npalm commented Jun 30, 2022

npalm commented Jul 6, 2022

mcaulifn commented Jul 6, 2022

npalm Jul 13, 2022

mcaulifn Jul 18, 2022

npalm Aug 3, 2022

mcaulifn Aug 4, 2022 •

edited

Loading

mcaulifn Aug 4, 2022

mcaulifn Sep 15, 2022

npalm Nov 17, 2022

npalm Nov 17, 2022

github-actions bot commented Sep 4, 2022

npalm commented Sep 16, 2022

npalm commented Oct 14, 2022

github-actions bot commented Nov 14, 2022

npalm commented Nov 16, 2022

npalm commented Nov 16, 2022

npalm left a comment

npalm commented Nov 24, 2022

npalm left a comment

fix(runners): Namespace Application tag #2182

fix(runners): Namespace Application tag #2182

Conversation

mcaulifn commented Jun 22, 2022 • edited by npalm Loading

mcaulifn commented Jun 22, 2022

npalm commented Jun 23, 2022

mcaulifn commented Jun 23, 2022

npalm commented Jun 23, 2022

mcaulifn commented Jun 24, 2022

mcaulifn commented Jun 29, 2022

npalm commented Jun 30, 2022

npalm commented Jul 6, 2022

mcaulifn commented Jul 6, 2022

npalm Jul 13, 2022

Choose a reason for hiding this comment

mcaulifn Jul 18, 2022

Choose a reason for hiding this comment

npalm Aug 3, 2022

Choose a reason for hiding this comment

mcaulifn Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

mcaulifn Aug 4, 2022

Choose a reason for hiding this comment

mcaulifn Sep 15, 2022

Choose a reason for hiding this comment

npalm Nov 17, 2022

Choose a reason for hiding this comment

npalm Nov 17, 2022

Choose a reason for hiding this comment

github-actions bot commented Sep 4, 2022

npalm commented Sep 16, 2022

npalm commented Oct 14, 2022

github-actions bot commented Nov 14, 2022

npalm commented Nov 16, 2022

npalm commented Nov 16, 2022

npalm left a comment

Choose a reason for hiding this comment

npalm commented Nov 24, 2022

npalm left a comment

Choose a reason for hiding this comment

fix(runners): Namespace `Application` tag #2182

fix(runners): Namespace `Application` tag #2182

mcaulifn commented Jun 22, 2022 •

edited by npalm

Loading

mcaulifn Aug 4, 2022 •

edited

Loading