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 dnsConfig to runner deployments #764

Merged
merged 1 commit into from
Aug 31, 2021
Merged

Add dnsConfig to runner deployments #764

merged 1 commit into from
Aug 31, 2021

Conversation

sammcj
Copy link
Contributor

@sammcj sammcj commented Aug 25, 2021

Add ability to set dnsConfig for runner deployments.

As per #761 (comment)

Note: I've never touched any golang or helm so just sort of fumbled my way through this - I'm sure I've missed several things.

@sammcj
Copy link
Contributor Author

sammcj commented Aug 26, 2021

The test the failed isn't clear why it failed.

@mumoshu
Copy link
Collaborator

mumoshu commented Aug 26, 2021

@sammcj seems like you need to rerun make release to regenerate manifests and code!

@sammcj
Copy link
Contributor Author

sammcj commented Aug 26, 2021

Thanks @mumoshu, I couldn't see that step mentioned in the README.md or CONTRIBUTING.md.

@sammcj
Copy link
Contributor Author

sammcj commented Aug 27, 2021

Have rebased and pushed

@sammcj
Copy link
Contributor Author

sammcj commented Aug 29, 2021

The test failure doesn't sound related:

Summarizing 1 Failure:

[Fail] INTEGRATION: Inside of a new namespace when no existing resources exist [It] should create and scale user's repository runners only on pull_request event 
/home/runner/work/actions-runner-controller/actions-runner-controller/controllers/integration_test.go:850

Ran 12 of 12 Specs in 34.532 seconds
FAIL! -- 11 Passed | 1 Failed | 0 Pending | 0 Skipped

@mumoshu any chance you could point me in the right direction?

@mumoshu
Copy link
Collaborator

mumoshu commented Aug 29, 2021

That might be due to test flakiness. Let me rerun it.

@sammcj
Copy link
Contributor Author

sammcj commented Aug 31, 2021

Looks like it worked, do you think it looks like everything that's required?

@mumoshu
Copy link
Collaborator

mumoshu commented Aug 31, 2021

Yep, let's go ahead! Thanks for your contribution ☺️

@mumoshu mumoshu merged commit 0593125 into actions:master Aug 31, 2021
@sammcj sammcj deleted the dnsconfig branch August 31, 2021 01:11
Copy link

@guyjacksongit guyjacksongit left a comment

Choose a reason for hiding this comment

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

nice

@billimek
Copy link
Contributor

@sammcj has this worked as expected for you? I'm struggling at the moment and wondering if there is a configuration nuance that I'm missing.

With the following RunnerDeployment config snippet,

apiVersion: actions.summerwind.dev/v1alpha1
kind: RunnerDeployment
metadata:
  name: myorg-ephemeral-dns
  namespace: actions
spec:
  replicas: 1
  template:
    spec:
      organization: myorg
      labels:
        - myorg-onprem-dns
      dnsConfig:
        - options:
          - name: "ndots"
            value: "1"

... The Runners get created with the dnsConfig settings, but the associated pods do not. I've experimented with different indenting and even other dnsConfig type settings, but nothing seems to translate to a change to the pod spec configuration or the actual /etc/resolv.conf file being changed.

@billimek
Copy link
Contributor

I think we may be missing something like this in controllers/runner_controller.go, based on comparing this change with a similar change

	if len(runner.Spec.DnsConfig ) != 0 {
		pod.Spec.DnsConfig = runner.Spec.DnsConfig 
	}

Going to try to test locally and if that's the case submit a new PR.

@billimek
Copy link
Contributor

#1227 raised for the above, FYI

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.

4 participants