-
Notifications
You must be signed in to change notification settings - Fork 32
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
Initial code commit #2
Conversation
Signed-off-by: Michael Gfeller <[email protected]>
Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, please review the Layer5 Community Welcome Guide and sure to join the community Slack. |
I suggest we merge this and then we can start collaborating from there :-) |
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.
"init" - lgtm
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.
There may have a set of questions we need to clarify.
on: | ||
push: | ||
branches: | ||
- 'master' | ||
- '**' |
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.
Is this mean of all the PR would run the CI?
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.
It means that the CI runs on any branch one pushes to, especially also any branch in personal forks.
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.
If I remember right, Github decreased the number of CI of the repo, can these make 0 effect to us?
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.
I haven't thought about that (but heard about it too). As most branches exist in personal forks, I think it would not contribute to the Layer5 budget. Let's keep an eye on it. Would be interested to see whether there is a count somewhere that shows usage...
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.
https://github.com/pricing
2,000 Actions minutes/month
Free for public repositories
GitHub Actions usage is free for public repositories and self-hosted runners.
@@ -0,0 +1,11 @@ | |||
check: |
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.
Maybe we can add go mod tidy/verify
to the Makefile
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.
Good suggestion 👍
What about a target called verify
with go mod verify
?
And a target called tidy that does go mod tidy
, gofmt -w .
, goimports -w .
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.
Yep, we can add tidy verify golang-ci
at local test
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.
Created #5
// creates the namespace unless it is 'default', or it is a delete operation | ||
func (h *BaseHandler) CreateNamespace(isDelete bool, namespace string) error { | ||
if !isDelete && namespace != "default" { | ||
if err := h.createNamespace(context.TODO(), namespace); err != nil { |
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.
I saw many of the tools for deploying the manifest to the apiserver
with waiting-for-ready
(retry), maybe we can add the similar type of parameters too.
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.
Yes. It would be nice if you could create an issue for that, and/or point to one of those tools?
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.
I have created an issue in the meshery-linkerd
repo, I saw the structure at Helm v3 pkg/kube
if I remember right, may be we can research more deep with that.
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.
Created #6
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.
Thank you sir
return nil | ||
} | ||
|
||
func (h *BaseHandler) splitYAML(yamlContents string) ([]string, error) { |
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.
This function may not work well on linkerd-adapter
maybe we can optimize it.
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.
Out of curiosity, why might it not work well?
Also, I'm slightly surprised that there is no library that allows to split a multi line string on arbitrary delimiter - at least I couldn't find any. Or specifically for YAML.
We should probably borrow the tests from apimachinery as well, especially if we optimize this function.
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.
As we know the Linkerd2 with many of CRDs, while we split the yaml
, there is some resource missed and made the deployments belong to Linkerd2 like tap
or other may not run to expected status
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.
Yep, I want try to find something interesting in the kubectl
repo, but it's not happened. If I remember right, I saw yaml
tools in apimachinery
but may not includes spilt.
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 clarification about the Linkerd2 issue.
Yes, it's from apimachinery (https://github.com/kubernetes/apimachinery/blob/master/pkg/util/yaml/decoder.go) Unfortunately, splitYAML isn't public.
import ( | ||
"fmt" | ||
|
||
"github.com/layer5io/gokit/errors" |
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.
gokit
may not in stable?
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.
Yes, it might not be. However, it is versioned and I think it is good to use it, then gain experience with it.
The current repository is most probably not stable yet either.
Just noted that I need to a tag to this repository here too, I suggest v0.1.0-alpha.1
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.
It's there some reason we must needs to use github.com/layer5io/gokit/errors
repo? Is there any plan?
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.
The reason was that gokit is used in the kuma adapter, and this code is based on kuma adapter as well.
Are you referring to errors
in particular, or gokit
?
I agree we should discuss the plan. There is some discussion here https://docs.google.com/document/d/1b8JAMzr3Rntu7CudRaYv6r6ccACJONAB5t7ISCaPNuA/edit#heading=h.blih70a9hxp and we could also discuss in the issue you created meshery/meshkit#1
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.
Ok, No problem.
@Aisuko thanks for your feedback. I added some comments. |
Signed-off-by: Michael Gfeller [email protected]
Description
This PR fixes #1
Notes for Reviewers
Signed commits