-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
@rambohe-ch: GitHub didn't allow me to assign the following users: your_reviewer. Note that only openyurtio members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
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. |
/assign @huiwq1990 @Congrool |
@rambohe-ch: GitHub didn't allow me to assign the following users: huiwq1990, congrool. Note that only openyurtio members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
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. |
3418814
to
55345b3
Compare
Codecov Report
@@ Coverage Diff @@
## master #72 +/- ##
========================================
Coverage ? 7.78%
========================================
Files ? 17
Lines ? 1901
Branches ? 0
========================================
Hits ? 148
Misses ? 1724
Partials ? 29
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. |
a75342b
to
b4f8c92
Compare
.github/workflows/ci.yaml
Outdated
jobs: | ||
|
||
check-license: | ||
if: github.repository == 'openyurtio/yurt-app-manager' |
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.
Why add the if 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.
only check openyurtio/yurt-app-manager repo, skip repos like rambohe-ch/yurt-app-manager
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 think it's better for contributors to trigger the github actions on their own forked repo to debug first before submitting a pr to the openyurt repo.
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 think it's better for contributors to trigger the github actions on their own forked repo to debug first before submitting a pr to the openyurt repo.
Yes, I agree.
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.
fixed.
@@ -115,6 +114,9 @@ GOLANGCI_LINT = $(shell pwd)/bin/golangci-lint | |||
golangci-lint: ## Download golangci-lint locally if necessary. | |||
$(call go-get-tool,$(GOLANGCI_LINT),github.com/golangci/golangci-lint/cmd/golangci-lint@v1.42.1) | |||
|
|||
lint: golangci-lint ## Run go lint against code. | |||
$(GOLANGCI_LINT) run -v |
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 seems that we will install golangci-lint@1.42.1.
But we run golangci-lint@1.31.1 in github actions.
We should make them the same version.
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.
fixed.
@@ -17,6 +17,7 @@ limitations under the License. | |||
|
|||
package refmanager | |||
|
|||
/* |
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.
Why we comment all this file?
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.
all of unit test cases can not pass, and i can not fix in a short time. so comment it temporarily.
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.
Could we create an issue to trace 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.
@Congrool yes, we are improving unit tests of yurt-app-manager now, so new test cases will be added soon.
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.
Well, I mean we can currently create an issue as a memo about the left problems of this pr, to avoid forgeting it.
Currently, we comment
And when we finally complete these problems, we can close this issue.
b4f8c92
to
7db4e60
Compare
/lgtm |
@Congrool: changing LGTM is restricted to collaborators In response to this:
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 APPROVED This pull-request has been approved by: Congrool, rambohe-ch The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind enhancement
What this PR does / why we need it:
add ci workflow for yut-app-manager
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
other Note