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

add app channel address flag for run #1283

Merged
merged 1 commit into from
May 31, 2023

Conversation

mukundansundar
Copy link
Collaborator

Description

Please explain the changes you've made

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #[issue number]

Checklist

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

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #1283 (060f159) into release-1.11 (c65d816) will not change coverage.
The diff coverage is n/a.

❗ Current head 060f159 differs from pull request most recent head 83ea992. Consider uploading reports for the commit 83ea992 to get more accurate results

@@              Coverage Diff              @@
##           release-1.11    #1283   +/-   ##
=============================================
  Coverage         26.87%   26.87%           
=============================================
  Files                39       39           
  Lines              3873     3873           
=============================================
  Hits               1041     1041           
  Misses             2758     2758           
  Partials             74       74           
Impacted Files Coverage Δ
pkg/standalone/run.go 0.00% <ø> (ø)

utils/utils.go Outdated
@@ -48,6 +48,9 @@ const (

windowsOsType = "windows"
homeDirPrefix = "~/"

// DefaultChannelAddress is the default local network address that user application listen on.
DefaultChannelAddress = "127.0.0.1"
Copy link
Member

Choose a reason for hiding this comment

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

Not that this is likely to change, but can this be used from the dapr/dapr package? Its already defined there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If possible I do not want to create another dependency in runtime. Can we move certain constants to kit and let both runtime and CLI depend on that instead?

Copy link
Member

@yaron2 yaron2 May 18, 2023

Choose a reason for hiding this comment

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

What's the issue with adding a package from dapr/dapr? In Go, as long as you don't have code inside internal, it's perfectly fine to use it from other referenced code. This ensures we have consistency and not minimize chances of non-synced changes. If anything, I would advocate for the CLI to rely on dapr/dapr packages as much as it possibly can.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Importing github.com/dapr/pkg/runtime into utils.go for using the constant defined there started creating issue wrt duplicate protobuf registration https://github.com/dapr/cli/actions/runs/5129434834/jobs/9227056977 associated commit 0381466

for now hardcoding that constant to 127.0.0.1 directly in CLI itself.

@mukundansundar mukundansundar marked this pull request as ready for review May 25, 2023 05:59
@mukundansundar mukundansundar requested review from a team as code owners May 25, 2023 05:59
@mukundansundar mukundansundar marked this pull request as draft May 31, 2023 04:23
@yaron2
Copy link
Member

yaron2 commented May 31, 2023

Please also update the README file with an example.

@mukundansundar
Copy link
Collaborator Author

Please also update the README file with an example.

I am also writing an E2E for the same ... will do it

@mukundansundar mukundansundar changed the base branch from master to release-1.11 May 31, 2023 05:42
@mukundansundar
Copy link
Collaborator Author

Please also update the README file with an example.

I am also writing an E2E for the same ... will do it

Added as part of usage help.

@mukundansundar mukundansundar marked this pull request as ready for review May 31, 2023 07:33
utils/utils.go Outdated Show resolved Hide resolved
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Copy link
Contributor

@pravinpushkar pravinpushkar left a comment

Choose a reason for hiding this comment

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

LGTM

@mukundansundar mukundansundar merged commit c3f08b0 into dapr:release-1.11 May 31, 2023
@mukundansundar mukundansundar deleted the add-app-channel branch May 31, 2023 12:21
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.

3 participants