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

*: adapt needs for operator support #76

Merged
merged 4 commits into from
Sep 9, 2022
Merged

*: adapt needs for operator support #76

merged 4 commits into from
Sep 9, 2022

Conversation

xhebox
Copy link
Collaborator

@xhebox xhebox commented Sep 8, 2022

Signed-off-by: xhe xw897002528@gmail.com

What problem does this PR solve?

Issue Number: ref #69

Problem Summary: A big PR including changes needed to adapt tidb-operator

What is changed and how it works:

  1. github actions now detects version by go.mod
  2. split parts from pkg into /lib. Otherwise tidb-operator is not able to import weirctl due to the complex dependency conflicts.
  3. Makefile and dockerfile improvement.
  4. server.Run returns no error
  5. Added --pub_addr for weirproxy to expose the correct advertise address in k8s, simplify xxx-urls config, too
  6. Removed unnecessary labels in metrics, but currently there is no usefull metrics for weirproxy, see Support tidb-operator and dev tier #69
  7. Removed pingcap/log in util/security

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
    Tested with k8s
  • No code

Notable changes

  • Has configuration change
  • Has HTTP API interfaces change (Don't forget to add the declarative for API)
  • Has weirctl change
  • Other user behavior changes

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
@@ -42,13 +42,13 @@ func NewNamespacesFromDir(nsdir string) (*NamespaceDir, error) {
nspath: make(map[string]string),
}

yamlFiles, err := listAllYamlFiles(c.dir)
nFiles, err := listAllFiles(c.dir, ".toml")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why change to toml files this time? Is it required by the operator?

Copy link
Collaborator Author

@xhebox xhebox Sep 8, 2022

Choose a reason for hiding this comment

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

Kind of. Operator has built-in support for json and toml but not yaml. Weird enough that TiDB actually uses toml config file.... So config operator be like type no-lint/no-highlight toml in yaml files.
Anyway, dynamic namespace will not be done by using configuration file(can not restart the proxy). Also this is duplicated with weirctl.
It is not used at all, I removed this.

lib/config/file.go Outdated Show resolved Hide resolved
pkg/server/server.go Outdated Show resolved Hide resolved
sigs.k8s.io/yaml v1.2.0 // indirect
)

replace github.com/pingcap/TiProxy/lib => ./lib
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why?

Copy link
Collaborator Author

@xhebox xhebox Sep 8, 2022

Choose a reason for hiding this comment

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

Same reason as parser as sub module of tidb. Can not test without pushing lib first. Golang reall sucks at multimodule and github workflow.

Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
@djshow832 djshow832 merged commit 06effc0 into main Sep 9, 2022
@djshow832 djshow832 deleted the test branch September 9, 2022 01:43
@xhebox xhebox mentioned this pull request Sep 9, 2022
8 tasks
@xhebox xhebox restored the test branch September 9, 2022 03:55
@xhebox xhebox deleted the test branch September 19, 2022 05:54
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.

2 participants