fix: when branch name is invalid, report a error message

[mangoGoForward]

What type of PR is this?

/kind bug
/kind hacktoberfest
/kind hacktoberfest-accepted

What this PR does / why we need it:

Before save resource, we should check branch name whether is valid, and build the label selector to query target PipelineRuns, we should check the branch name of request also.

Which issue(s) this PR fixes:

Fixes #104

[ks-ci-bot]

@mangoGoForward: The label(s) kind/hacktoberfest, kind/hacktoberfest-accepted cannot be applied, because the repository doesn't have them.

In response to this:

What type of PR is this?

/kind bug
/kind hacktoberfest
/kind hacktoberfest-accepted

What this PR does / why we need it:

Before save resource, we should check branch name whether is valid, and build the label selector to query target PipelineRuns, we should check the branch name of request also.

Which issue(s) this PR fixes:

Fixes #104

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.

[mangoGoForward]

/assign @LinuxSuRen

[codecov]

Codecov Report

Merging #328 (c842524) into master (9727751) will increase coverage by 0.04%.
The diff coverage is 87.50%.

@@            Coverage Diff            @@
##           master    #328      +/-   ##
=========================================
+ Coverage    7.70%   7.75%   +0.04%     
=========================================
  Files          94      94              
  Lines       21443   21449       +6     
=========================================
+ Hits         1652    1663      +11     
+ Misses      19667   19662       -5     
  Partials      124     124              

Flag	Coverage Δ
unittests	7.75% <87.50%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/kapis/devops/v1alpha3/pipeline/handler.go	0.00% <0.00%> (ø)
...lers/jenkins/pipelinerun/pipelinerun_controller.go	12.33% <100.00%> (+1.15%)	⬆️
...kg/kapis/devops/v1alpha3/pipeline/branch_filter.go	100.00% <100.00%> (ø)
pkg/kapis/devops/v1alpha3/pipelinerun/util.go	37.93% <100.00%> (+2.21%)	⬆️
...trollers/devopsproject/devopsproject_controller.go	40.62% <0.00%> (-2.61%)	⬇️
...rs/devopscredential/devopscredential_controller.go	36.48% <0.00%> (+3.37%)	⬆️
controllers/s2ibinary/s2ibinary_controller.go	38.00% <0.00%> (+5.00%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9727751...c842524. Read the comment docs.

[JohnNiang]

Thanks for your quick fixing.

/area devops
/cc @kubesphere/sig-devops

[JohnNiang]

I'm wondering if there's a better way to check the label, such as
I found a solution from the kubernetes project, we might be able to reuse it.

https://github.com/kubernetes/apimachinery/blob/a7973b273a7e6665f300fbadacab2f70fca0f3c3/pkg/util/validation/validation.go#L167

[ks-ci-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mangoGoForward
To complete the pull request process, please ask for approval from linuxsuren after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mangoGoForward]

I'm wondering if there's a better way to check the label, such as I found a solution from the kubernetes project, we might be able to reuse it.

https://github.com/kubernetes/apimachinery/blob/a7973b273a7e6665f300fbadacab2f70fca0f3c3/pkg/util/validation/validation.go#L167

Thanks a lot, I have changed, could you help me to review it?

[JohnNiang]

I'm wondering if there's a better way to check the label, such as I found a solution from the kubernetes project, we might be able to reuse it.

https://github.com/kubernetes/apimachinery/blob/a7973b273a7e6665f300fbadacab2f70fca0f3c3/pkg/util/validation/validation.go#L167

Thanks a lot, I have changed, could you help me to review it?

Nice job!

But I'm curious about is, why not use k8s.io/apimachinery/pkg/util/validation.IsValidLabelValue function directly, but handwritten. : P

[mangoGoForward]

I'm wondering if there's a better way to check the label, such as I found a solution from the kubernetes project, we might be able to reuse it.

https://github.com/kubernetes/apimachinery/blob/a7973b273a7e6665f300fbadacab2f70fca0f3c3/pkg/util/validation/validation.go#L167

Thanks a lot, I have changed, could you help me to review it?

Nice job!

But I'm curious about is, why not use k8s.io/apimachinery/pkg/util/validation.IsValidLabelValue function directly, but handwritten. : P

The penny drops, and the code is more elegant.

[JohnNiang]

/lgtm

[LinuxSuRen]

Thanks for your effort. But I still meet some issues. Please check the following screenshot:

I test it with the following images:

kubespheredev/devops-apiserver:dev-v3.2.0-rc.1-c842524

kubespheredev/devops-controller:dev-v3.2.0-rc.1-c842524

[LinuxSuRen]

IMO, if there's no perfect solution for some special character in the git branch, then we can exclude (AKA do not show) them. So, we can claim that no support for those git branches which are Chinese words.

How does that sound like? @mangoGoForward @JohnNiang

[mangoGoForward]

IMO, if there's no perfect solution for some special character in the git branch, then we can exclude (AKA do not show) them. So, we can claim that no support for those git branches which are Chinese words.

How does that sound like? @mangoGoForward @JohnNiang

I think so, when I try to translate git branch name to english when it is invalid, but found that I don't know the words is Chinese, and if it is other language such as Japanese, it also can not solve.

[LinuxSuRen]

So, I'd like to suggest we exclude those branches that have invalid characters (not match with the Kubernetes label naming rule). I believe the following code lines:

ks-devops/pkg/kapis/devops/v1alpha3/pipeline/handler.go

Line 51 in e20f3ba

if err := json.Unmarshal([]byte(branchesJSON), &branches); err != nil {

[mangoGoForward]

So, I'd like to suggest we exclude those branches that have invalid characters (not match with the Kubernetes label naming rule). I believe the following code lines:

ks-devops/pkg/kapis/devops/v1alpha3/pipeline/handler.go

Line 51 in e20f3ba

if err := json.Unmarshal([]byte(branchesJSON), &branches); err != nil {

Done

[LinuxSuRen]

/lgtm

Thanks for your patient.

I'm wondering if you have the same email address between GitHub and your git config. Your avatars' color is gray, and it might be caused by the email setting. This would block the merge process if you don't mind it.

By the way, you can join our WeChat group of the DevOps SIG if you are interested in it. My WeChat ID is linuxsuren.

[mangoGoForward]

/lgtm

Thanks for your patient.

I'm wondering if you have the same email address between GitHub and your git config. Your avatars' color is gray, and it might be caused by the email setting. This would block the merge process if you don't mind it.

By the way, you can join our WeChat group of the DevOps SIG if you are interested in it. My WeChat ID is linuxsuren.

Thanks, Both use same email address between Github and git config correctly. And I'm glad to join the group.

[JohnNiang]

Hi @mangoGoForward , I've left a potential solution in #104 (comment). Please take a moment to have a look.

[LinuxSuRen]

Hi @mangoGoForward , I've left a potential solution in #104 (comment). Please take a moment to have a look.

hi @JohnNiang , I prefer to take your solution in a new PR instead of this one. This PR can avoid the necessary errors with small changes. But it will make big changes with your solution.

[JohnNiang]

Hi @mangoGoForward , I've left a potential solution in #104 (comment). Please take a moment to have a look.

hi @JohnNiang , I prefer to take your solution in a new PR instead of this one. This PR can avoid the necessary errors with small changes. But it will make big changes with your solution.

Agree with you.

/lgtm