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

feat: add amazonec2 userdata for docker machines #608

Merged
merged 7 commits into from
Jan 12, 2023

Conversation

FelikZ
Copy link
Contributor

@FelikZ FelikZ commented Dec 16, 2022

Description

Introducing support for amazonec2-userdata flag that is not mentioned on driver readme page, but it is there and useful.

For example, it can be used to introduce swap for the runner instance or any other user command.

Migrations required

NO - this is optional flag and defaults to empty string.

Verification

I have verified on my own runners cluster that user script that I've added works (it creates swap in my case).

module "runner" {
  # source  = "npalm/gitlab-runner/aws"
  # version = "5.5.0"

  # ...

  runners_userdata = templatefile("${path.module}/../../templates/swap.tpl", {
    swap_size = "512"
  })
}

@FelikZ
Copy link
Contributor Author

FelikZ commented Dec 16, 2022

But I have no idea how to auto-generate README.md, I think this flag should be documented. (manually edit?)
UPD: I see there is github action for that CI / Auto update terraform docs (pull_request), neat.

Copy link
Collaborator

@kayman-mk kayman-mk left a comment

Choose a reason for hiding this comment

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

Good PR! Just 2 little remarks.

template/gitlab-runner.tpl Outdated Show resolved Hide resolved
template/runner-config.tpl Outdated Show resolved Hide resolved
@kayman-mk kayman-mk changed the title Add amazonec2 userdata feat: add amazonec2 userdata for docker machines Jan 6, 2023
@kayman-mk kayman-mk self-requested a review January 6, 2023 14:32
kayman-mk
kayman-mk previously approved these changes Jan 6, 2023
@kayman-mk
Copy link
Collaborator

@npalm See my comment above. Better you check this as well.

npalm
npalm previously requested changes Jan 7, 2023
Copy link
Collaborator

@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.

@FelikZ thanks looks good! Please can you add the example. Which you mention in the PR to either the Readme. Or as comment in the default example. I would prefer the last one (example)

@FelikZ
Copy link
Contributor Author

FelikZ commented Jan 12, 2023

@npalm please have a look. Thanks

@kayman-mk
Copy link
Collaborator

Did you do a quick check without using the new variable runners_userdata to make sure that the agent is still able to start?

@kayman-mk
Copy link
Collaborator

Ok, it's working. Had a test environment available and deployed your changes. It is working without runners_userdata.

@kayman-mk kayman-mk merged commit be789ff into cattle-ops:main Jan 12, 2023
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.

None yet

3 participants