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.

Sign up for GitHub

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

Jump to bottom

Fix: removes default parameter name for terraform provider #5468

Merged
merged 2 commits into from
Feb 16, 2023
Merged

Fix: removes default parameter name for terraform provider #5468

merged 2 commits into from
Feb 16, 2023

Conversation

afzalbin64
Copy link
Contributor

@afzalbin64 afzalbin64 commented Feb 9, 2023 •
edited
Loading

Description of your changes

Fixes #5427

I have:

  • Read and followed KubeVela's contribution process.
  • Related Docs updated properly. In a new feature or configuration option, an update to the documentation is necessary.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Special notes for your reviewer

@codecov
Copy link

codecov bot commented Feb 9, 2023 •
edited
Loading

Codecov Report

Base: 61.22% // Head: 55.23% // Decreases project coverage by -6.00% ⚠️

Coverage data is based on head (dc1181e) compared to base (c2de316).
Patch has no changes to coverable lines.

❗ Current head dc1181e differs from pull request most recent head 51c2650. Consider uploading reports for the commit 51c2650 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5468      +/-   ##
==========================================
- Coverage   61.22%   55.23%   -6.00%     
==========================================
  Files         310      175     -135     
  Lines       47160    26120   -21040     
==========================================
- Hits        28875    14428   -14447     
+ Misses      15291    10262    -5029     
+ Partials     2994     1430    -1564
Flag Coverage Δ
apiserver-e2etests ?
apiserver-unittests ?
core-unittests 55.23% <ø> (-0.03%) ⬇️
e2e-multicluster-test ?
e2e-rollout-tests ?
e2etests ?

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

Impacted Files Coverage Δ
pkg/config/factory.go 64.37% <ø> (-5.76%) ⬇️
pkg/workflow/workflow.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/controller/utils/actions.go 0.00% <0.00%> (-80.00%) ⬇️
pkg/resourcekeeper/containsresources.go 0.00% <0.00%> (-80.00%) ⬇️
pkg/auth/identity.go 0.00% <0.00%> (-76.06%) ⬇️
...v/v1alpha2/componentdefinition/mutating_handler.go 0.00% <0.00%> (-74.29%) ⬇️
pkg/oam/auxiliary.go 15.21% <0.00%> (-71.74%) ⬇️
pkg/auth/privileges.go 0.00% <0.00%> (-69.39%) ⬇️
pkg/utils/json.go 0.00% <0.00%> (-57.15%) ⬇️
pkg/utils/jwt.go 0.00% <0.00%> (-55.00%) ⬇️
... and 222 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

chivalryq
chivalryq reviewed Feb 9, 2023
View reviewed changes
Copy link
Member

@chivalryq chivalryq left a comment

Choose a reason for hiding this comment

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

The user prompt should only show when the config will be override. User shouldn't be asked twice when creating a new config.

@afzal442
removes default name for terraform provider
92e7bf8 
Signed-off-by: afzalbin64 <afzal442@gmail.com>
@afzalbin64 afzalbin64 mentioned this pull request Feb 13, 2023
Merged
9 tasks
@afzalbin64 afzalbin64 changed the title Feat: adds prompts to ask user if the command will update the config Fix: removes default parameter name for terraform provider Feb 13, 2023
@chivalryq
Copy link
Member

After offline talk with @afzalbin64 . #5427 should be fixed by remove default provider name ("default") in config templates. This PR can be closed.
See: kubevela/catalog#638

@afzalbin64 afzalbin64 requested review from chivalryq and removed request for zzxwill, StevenLeiZhang and charlie0129 February 14, 2023 04:26
@chivalryq
Copy link
Member

Please fix the test.

@afzalbin64
Copy link
Contributor Author

Thanks. I will take care.
make test: https://github.com/kubevela/kubevela/actions/runs/4161269005/jobs/7199053258
make unit-test-apiserver: https://github.com/kubevela/kubevela/actions/runs/4161269010/jobs/7199053409

@afzal442
fixes minor typos
51c2650 
Signed-off-by: afzalbin64 <afzal442@gmail.com>

undo the changes to typo

fixes minor typos in go_test files

updates config_test to support name as required param
@afzalbin64
Copy link
Contributor Author

cc @chivalryq @wonderflow

FogDong
FogDong reviewed Feb 16, 2023
View reviewed changes
Copy link
Member

@FogDong FogDong left a comment

Choose a reason for hiding this comment

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

Should this be backported to 1.7?

@chivalryq chivalryq added the backport release-1.7 add this label will automatically backport this PR to release-1.7 branch label Feb 16, 2023
@chivalryq chivalryq merged commit 39c24cf into kubevela:master Feb 16, 2023
@github-actions github-actions bot mentioned this pull request Feb 16, 2023
Merged
@github-actions
Copy link

Successfully created backport PR #5516 for release-1.7.

@afzalbin64 afzalbin64 deleted the add-userConfirm-velacreate branch February 16, 2023 07:05
zhaohuiweixiao pushed a commit to zhaohuiweixiao/kubevela that referenced this pull request Mar 7, 2023
@afzalbin64 @zhaohuiweixiao
Fix: removes default parameter name for terraform provider (kubevel…
2fd9817 
…a#5468)

Co-authored-by: afzalbin64 <afzal442@gmail.com>
Fixes kubevela#5427
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FogDong FogDong FogDong approved these changes

@Somefive Somefive Somefive approved these changes

@chivalryq chivalryq chivalryq approved these changes

@barnettZQG barnettZQG Awaiting requested review from barnettZQG barnettZQG is a code owner

@yangsoon yangsoon Awaiting requested review from yangsoon

@wonderflow wonderflow Awaiting requested review from wonderflow wonderflow is a code owner

@leejanee leejanee Awaiting requested review from leejanee leejanee is a code owner

@jefree-cat jefree-cat Awaiting requested review from jefree-cat jefree-cat is a code owner

Assignees
No one assigned
Labels
backport release-1.7 add this label will automatically backport this PR to release-1.7 branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Double check on config update
5 participants
@afzalbin64 @chivalryq @Somefive @FogDong @afzal442