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.

Sign up for GitHub

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

Jump to bottom

feat: allow setting VPC and subnets per runner #3467

Merged
merged 6 commits into from
Feb 12, 2024
Merged

feat: allow setting VPC and subnets per runner #3467

merged 6 commits into from
Feb 12, 2024

Conversation

taharah
Copy link
Contributor

@taharah taharah commented Sep 6, 2023 •
edited
Loading

This allows passing in a different VPC and subnet IDs for each runner config and falling back to the "global" value set via the existing vpc_id and subnet_ids variables.

@taharah taharah force-pushed the multi-runner-vpc branch 3 times, most recently from 507ad56 to 66389bf Compare September 6, 2023 02:29
@taharah
Copy link
Contributor Author

taharah commented Sep 13, 2023

Rebased the PR on the latest changes as well as pushed up a fix after testing this change internally.

@pbarnajc pbarnajc mentioned this pull request Oct 3, 2023
Closed
@github-actions GitHub Actions
Copy link
Contributor

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.

@github-actions github-actions bot added Stale and removed Stale labels Nov 14, 2023
@taharah taharah requested a review from npalm December 3, 2023 18:44
@dvarchev
Copy link

dvarchev commented Jan 2, 2024

Hi, I've noticed that this PR seems ready for merging. Could you kindly share if there's an expected timeline for the merge? Thanks for your great work on this!

@npalm
Copy link
Member

npalm commented Jan 3, 2024

Hi, I've noticed that this PR seems ready for merging. Could you kindly share if there's an expected timeline for the merge? Thanks for your great work on this!

Need to check again, thought there was some issue with the examples.

@taharah
Copy link
Contributor Author

taharah commented Jan 5, 2024

Need to check again, thought there was some issue with the examples.

Examples should be fixed now albeit with a solution that is less ideal than I would like. I'll take a look at fixing the failing tests tomorrow morning.

@taharah
Copy link
Contributor Author

taharah commented Jan 5, 2024

@npalm I've fixed the failing tests as well as rebased my branch.

npalm
npalm requested changes Jan 11, 2024
View reviewed changes
Copy link
Member

@npalm npalm left a comment

Choose a reason for hiding this comment

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

@taharah thanks for all the work. Change looks in general good and is working. However I prefer a single VPC in the example that we use as default for testing. I have provided suggestion to remove the second base.

examples/multi-runner/main.tf Outdated Show resolved Hide resolved
modules/runners/.terraform.lock.hcl Outdated Show resolved Hide resolved
modules/multi-runner/.terraform.lock.hcl Outdated Show resolved Hide resolved
examples/multi-runner/main.tf Outdated Show resolved Hide resolved
@taharah
Copy link
Contributor Author

taharah commented Jan 12, 2024

@npalm comments have been addressed and I have rebased to include the latest changes as well.

@effata
Copy link

effata commented Feb 9, 2024

@npalm This PR seems to have been ready for merge for a month now, any chance of getting it merged soon? It solves a very concrete use case for us, and I've been running a local fork of this PR for a while and it seems to work great.
Also, thanks for the great work with this module!

@npalm
Copy link
Member

npalm commented Feb 9, 2024

@npalm This PR seems to have been ready for merge for a month now, any chance of getting it merged soon? It solves a very concrete use case for us, and I've been running a local fork of this PR for a while and it seems to work great. Also, thanks for the great work with this module!

Sorry, but last weeks were busy. Will catch up with the PR asap.

npalm
npalm approved these changes Feb 12, 2024
View reviewed changes
Copy link
Member

@npalm npalm left a comment

Choose a reason for hiding this comment

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

@taharah thx, checked all good

@npalm npalm merged commit 1288c81 into philips-labs:main Feb 12, 2024
35 checks passed
@forest-releaser forest-releaser bot mentioned this pull request Feb 12, 2024
Merged
@taharah
Copy link
Contributor Author

taharah commented Feb 12, 2024

@taharah thx, checked all good

Thank you!

@taharah taharah deleted the multi-runner-vpc branch February 12, 2024 13:34
npalm pushed a commit that referenced this pull request Feb 12, 2024
@forest-releaser
chore(main): release 5.7.0 (#3739)
5fd1973 
🤖 I have created a release *beep* *boop*
---


##
[5.7.0](v5.6.3...v5.7.0)
(2024-02-12)


### Features

* allow setting VPC and subnets per runner
([#3467](#3467))
([1288c81](1288c81))


### Bug Fixes

* Correct typo in README.md
([#3758](#3758))
([7186c1c](7186c1c))
* **images:** avoid wrong AMI selected for ubuntu jammy
([#3747](#3747))
([595aec9](595aec9))
* **lambda:** bump @aws-lambda-powertools/logger from 1.17.0 to 1.18.0
in /lambdas
([#3754](#3754))
([98131ff](98131ff))
* **lambda:** bump axios from 1.6.2 to 1.6.7 in /lambdas
([#3755](#3755))
([80a34bd](80a34bd))
* **lambda:** bump the aws group in /lambdas with 5 updates
([#3730](#3730))
([7854a5f](7854a5f))
* **lambda:** bump the aws group in /lambdas with 5 updates
([#3743](#3743))
([7ca40ef](7ca40ef))
* **lambda:** bump the aws group in /lambdas with 5 updates
([#3753](#3753))
([9f3aa68](9f3aa68))
* windows userdata does not support gzip
([#3759](#3759))
([b74df54](b74df54))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: forest-releaser[bot] <80285352+forest-releaser[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuptaNavdeep1983 GuptaNavdeep1983 GuptaNavdeep1983 left review comments

@maartenvanderhoef maartenvanderhoef maartenvanderhoef left review comments

@npalm npalm npalm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@taharah @dvarchev @npalm @effata @maartenvanderhoef @GuptaNavdeep1983