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

chore(helm): allow install to namespace with numbers only #1494

Merged
merged 4 commits into from
Feb 6, 2021

Conversation

sedlakr
Copy link
Contributor

@sedlakr sedlakr commented Feb 5, 2021

What problem does this PR solve?

fixes #1493

What is changed and how does it work?

All used namespace vars are quoted to allow use number in namespace only

Checklist

Tests

  • Unit test
  • E2E test
  • Manual test (add detailed scripts or steps below)
  • No code

Does this PR introduce a user-facing change?

NONE

@codecov-io
Copy link

codecov-io commented Feb 5, 2021

Codecov Report

Merging #1494 (2256295) into master (7e9ff3f) will decrease coverage by 3.74%.
The diff coverage is 59.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1494      +/-   ##
==========================================
- Coverage   55.78%   52.03%   -3.75%     
==========================================
  Files          68       80      +12     
  Lines        4383     5104     +721     
==========================================
+ Hits         2445     2656     +211     
- Misses       1768     2178     +410     
- Partials      170      270     +100     
Impacted Files Coverage Δ
api/v1alpha1/common_types.go 0.00% <0.00%> (ø)
api/v1alpha1/common_webhook.go 100.00% <ø> (ø)
api/v1alpha1/dnschaos_type.go 0.00% <0.00%> (ø)
api/v1alpha1/dnschaos_webhook.go 0.00% <0.00%> (ø)
api/v1alpha1/httpchaos_types.go 0.00% <0.00%> (ø)
api/v1alpha1/iochaos_types.go 0.00% <ø> (-40.00%) ⬇️
api/v1alpha1/jvmchaos_webhook.go 0.00% <0.00%> (ø)
api/v1alpha1/kernelchaos_types.go 0.00% <ø> (-20.00%) ⬇️
api/v1alpha1/kernelchaos_webhook.go 100.00% <ø> (+14.81%) ⬆️
api/v1alpha1/kinds.go 27.27% <ø> (+0.60%) ⬆️
... and 131 more

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 61ed59d...2256295. Read the comment docs.

@fewdan
Copy link
Member

fewdan commented Feb 5, 2021

Looks like you need to deal with CI and DCO first? Thank you.

fixes chaos-mesh#1493

Signed-off-by: Radek Sedlák <r.sedlak@quadient.com>
fixes chaos-mesh#1493

Signed-off-by: Radek Sedlák <r.sedlak@quadient.com>
@sedlakr
Copy link
Contributor Author

sedlakr commented Feb 5, 2021

Hi, CI and DCO fixed

cwen0
cwen0 previously approved these changes Feb 5, 2021
Copy link
Member

@cwen0 cwen0 left a comment

Choose a reason for hiding this comment

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

@sedlakr Thanks for your contribution! LGTM

fixes chaos-mesh#1493

Signed-off-by: Radek Sedlák <r.sedlak@quadient.com>
@sedlakr
Copy link
Contributor Author

sedlakr commented Feb 5, 2021

@cwen0 one more fix on targetNamespace

fixes chaos-mesh#1493

Signed-off-by: Radek Sedlák <r.sedlak@quadient.com>
Copy link
Member

@fewdan fewdan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@g1eny0ung g1eny0ung left a comment

Choose a reason for hiding this comment

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

LGTM

@g1eny0ung
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@g1eny0ung g1eny0ung changed the title Allow install to namespace with numbers only e.g. helm install -n 12345 chore(helm): allow install to namespace with numbers only Feb 6, 2021
@ti-srebot
Copy link
Contributor

@sedlakr merge failed.

@g1eny0ung
Copy link
Member

/run-all-tests

1 similar comment
@g1eny0ung
Copy link
Member

/run-all-tests

@g1eny0ung g1eny0ung merged commit cd61d2e into chaos-mesh:master Feb 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't be installed to namespace with numbers only
6 participants