Skip to content

Run: pass missing env variables#860

Merged
yaron2 merged 2 commits intodapr:masterfrom
anhldbk:correct-env-variables
Jan 3, 2022
Merged

Run: pass missing env variables#860
yaron2 merged 2 commits intodapr:masterfrom
anhldbk:correct-env-variables

Conversation

@anhldbk
Copy link
Copy Markdown
Contributor

@anhldbk anhldbk commented Dec 22, 2021

Description

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Add tests for CLI (run command) to validate env variables
  • Modify code to pass the missing variables
  • Refactor code (run.go can be cleaner)
  • Update documentation: Env variables inconsistency docs#1835

@anhldbk anhldbk requested review from a team as code owners December 22, 2021 09:01
- In the past, I openned this PR: dapr#770
- DCO had some complaints.
- I incorrectly rebased.
- This PR is made by forking the master branch, modifying related files

Signed-off-by: Andy Le <anhldbk@gmail.com>
@anhldbk anhldbk force-pushed the correct-env-variables branch from e596e52 to 0a6bd76 Compare December 22, 2021 09:02
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 22, 2021

Codecov Report

Merging #860 (6833cb9) into master (64c303d) will increase coverage by 0.91%.
The diff coverage is 63.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #860      +/-   ##
==========================================
+ Coverage   22.32%   23.23%   +0.91%     
==========================================
  Files          29       29              
  Lines        1550     1588      +38     
==========================================
+ Hits          346      369      +23     
- Misses       1163     1169       +6     
- Partials       41       50       +9     
Impacted Files Coverage Δ
pkg/standalone/run.go 59.74% <63.90%> (+0.24%) ⬆️

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 64c303d...6833cb9. Read the comment docs.

@anhldbk
Copy link
Copy Markdown
Contributor Author

anhldbk commented Dec 22, 2021

@yaron2 PTAL

@yaron2
Copy link
Copy Markdown
Member

yaron2 commented Dec 22, 2021

LGTM overall.

@mukundansundar can you please take one last look before we merge?

@anhldbk
Copy link
Copy Markdown
Contributor Author

anhldbk commented Dec 29, 2021

@mukundansundar I wish this PR is merged on Jan 1st 2022. Would you pls make my dream come true?

@mukundansundar
Copy link
Copy Markdown
Collaborator

Overall looks good ... But my question still remains as to whether we need to expose these variables as ENV vars ... that too without DAPR prefix ...

@anhldbk
Copy link
Copy Markdown
Contributor Author

anhldbk commented Jan 3, 2022

@mukundansundar Happy new year :)

For your question, I already made a PR in our docs. Pls see

@yaron2 yaron2 merged commit 092b93d into dapr:master Jan 3, 2022
@anhldbk
Copy link
Copy Markdown
Contributor Author

anhldbk commented Jan 4, 2022

@yaron2 @mukundansundar yay. You make my day. Thank you!

@artursouza artursouza added this to the v1.6 milestone Jan 21, 2022
@rabollin
Copy link
Copy Markdown

@mukundansundar - Please prioritize this to get into 1.6 CLI release.

imneov pushed a commit to imneov/dapr-cli that referenced this pull request Mar 18, 2022
- In the past, I openned this PR: dapr#770
- DCO had some complaints.
- I incorrectly rebased.
- This PR is made by forking the master branch, modifying related files

Signed-off-by: Andy Le <anhldbk@gmail.com>
Signed-off-by: imneov <grantliu@yunify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants