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

Update README.md #261

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update README.md #261

wants to merge 4 commits into from

Conversation

IanMoroney
Copy link

Updated readme as the instructions were missing details about enabling github app authentication when installing the helm chart instead of manually applying it.

Added more detailed instructions for the manual method.

Updated readme as the instructions were missing details about enabling github app authentication when installing the helm chart instead of manually applying it.

Added more detailed instructions for the manual method.
Added additional comment about tokenRef, increased visibility of replacing the org name in two places (maybe this should just be one instead of two?)
Removed maven example, as that was preventing the pod from being scheduled at all.
example included no sample settings.xml so the example is incomplete.
This way, the example will run first time without errors.
@IanMoroney IanMoroney marked this pull request as ready for review June 10, 2021 16:14
removed trailing colons
@@ -45,14 +47,6 @@ Create a secret called `github-runner-app` by executing the following command in
kubectl create secret generic github-runner-app --from-literal=GITHUB_APP_INTEGRATION_ID=<app_id> --from-file=GITHUB_APP_PRIVATE_KEY=<private_key>
```

Finally define the following on the operator deployment:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this should be removed as it's describing how it actually works (although helm abstracts that)

Copy link
Author

Choose a reason for hiding this comment

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

This value is already set in the deployment.yaml if you were to download it manually and apply it.
For the approach mentioned in this section of the readme, this value both doesn't need to be set and cannot be set, without downloading the helm chart and deploying it manually.

Copy link
Author

Choose a reason for hiding this comment

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

When trying to follow these instructions, I got held up by not being able to find how or where to define that secretRef. I then found that it didn't need to be set at all, so for clarity, it should not be in the readme

Choose a reason for hiding this comment

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

@davidkarlsen, what should happen to resolve this code review so it can get merged into main?

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.

3 participants