-
Notifications
You must be signed in to change notification settings - Fork 416
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
getting-started: create webhook ExternalName vs GitHubDomain #936
Conversation
small fix in getting started doc
@HerrmannHinz: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @HerrmannHinz. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
@HerrmannHinz: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@HerrmannHinz: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Issues go stale after 90d of inactivity. /lifecycle stale Send feedback to tektoncd/plumbing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the doc update @HerrmannHinz and apologies for the review coming SO FAR after the PR was opened 🙏
@@ -166,7 +166,7 @@ minimum. | |||
- GitHubOrg: The GitHub org you are using for this getting-started. | |||
- GitHubUser: Your GitHub username. | |||
- GitHubRepo: The repo we will be using for this example. | |||
- ExternalDomain: Update this to be the to something other then `demo.iancoffey.com` | |||
- GitHubDomain: Update this to be the to something other then `demo.iancoffey.com` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm it seems like these are both important to the way the task works, but do different jobs - ExternalDomain is required, so im not sure it makes sense to completely remove it from these docs (https://github.com/tektoncd/triggers/blob/a545f76e7f0021ee13a1c22b6703dae18d2639fa/docs/getting-started/create-webhook.yaml)
if anything we probably want to add a bullet for GitHubDomain - explaining that you may need to override it if you're using github enterprise
@@ -19,7 +19,9 @@ spec: | |||
value: token | |||
- name: GitHubSecretStringKey | |||
value: secret | |||
- name: GitHubDomain | |||
value: git.corp.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im not sure we want to add this by default here - maybe we could have it commented out? e.g.
# If you are using github enterprise, uncomment this line <more context here>
# - name: GitHubDomain
# value: git.corp.com
These changes were initially submitted by @HerrmannHinz in tektoncd#936 and I requested a couple changes but since it's been so long since the PR was opened I wanted to make it easy for @HerrmannHinz and just make the changes for them. @HerrmannHinz if you want to make any changes please go ahead - and feel free to update the old PR or open a new one and I'll just close this one. Thank you!
Apologies again for taking so long to review this @HerrmannHinz - if it's easier I've opened #1151 with the changes I requested here and I'll close this PR for now since it needs a rebase - but if you want to pick this up again plz feel free to re-open this PR or make a new one and I'm happy to close mine. Thanks again! |
These changes were initially submitted by @HerrmannHinz in #936 and I requested a couple changes but since it's been so long since the PR was opened I wanted to make it easy for @HerrmannHinz and just make the changes for them. @HerrmannHinz if you want to make any changes please go ahead - and feel free to update the old PR or open a new one and I'll just close this one. Thank you!
small fix in getting started doc