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

make webhook configurable #529

Merged
merged 11 commits into from
May 30, 2019

Conversation

shuijing198799
Copy link
Contributor

What problem does this PR solve?

Make webhook configurable

What is changed and how it works?

Use shell script to patch namespace to webhook.yaml

Check List

Tests

  • E2E test
  • Stability test
  • Manual test (add detailed scripts or steps below)

Code changes

  • Has Go code change
  • Has scripts change

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Does this PR introduce a user-facing change?:


None

@shuijing198799
Copy link
Contributor Author

/run-e2e-tests

@shuijing198799
Copy link
Contributor Author

/run-e2e-tests

1 similar comment
@shuijing198799
Copy link
Contributor Author

/run-e2e-tests

@shuijing198799
Copy link
Contributor Author

/run-e2e-tests

@shuijing198799 shuijing198799 marked this pull request as ready for review May 29, 2019 07:51
@shuijing198799
Copy link
Contributor Author

@xiaojingchen @onlymellb PTAL

@shuijing198799 shuijing198799 requested a review from weekface May 29, 2019 07:52
while [[ $# -gt 0 ]]; do
case ${1} in
--namespace)
namespace="$2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
namespace="$2"
namespace="${2}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems the same ……

@weekface weekface requested a review from onlymellb May 29, 2019 08:38
@weekface
Copy link
Contributor

@onlymellb please take a look.

EOF
exit 1
}

namespace=default
while [[ $# -gt 0 ]]; do
Copy link
Contributor

Choose a reason for hiding this comment

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

Refer here to implement shell parameter parsing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

done


[ -z ${namespace} ] && namespace=default
Copy link
Contributor

Choose a reason for hiding this comment

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

Use namespace=${namespace:-default} instead.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should default to tidb-admin namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@shuijing198799
Copy link
Contributor Author

/run-e2e-tests

@shuijing198799
Copy link
Contributor Author

/run-e2e-tests

Copy link
Contributor

@onlymellb onlymellb left a comment

Choose a reason for hiding this comment

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

LGTM

@shuijing198799
Copy link
Contributor Author

/run-e2e-tests

@shuijing198799 shuijing198799 merged commit a9897a1 into pingcap:master May 30, 2019
yahonda pushed a commit that referenced this pull request Dec 27, 2021
* add enterprise usage

* address comments

* address comments

* address comments

* add tidb-binlog enterprise image
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.

5 participants