-
Notifications
You must be signed in to change notification settings - Fork 97
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
Installing the Dapr Helm Chart via the CLI #8033
Installing the Dapr Helm Chart via the CLI #8033
Conversation
@@ -39,11 +39,11 @@ func (r *Runner) enterClusterOptions(options *initOptions) error { | |||
if err != nil { | |||
return clierrors.MessageWithCause(err, "Unable to verify Radius installation.") | |||
} | |||
options.Cluster.Install = !state.Installed | |||
options.Cluster.Install = !state.RadiusInstalled || !state.DaprInstalled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a test for this new case?
pkg/cli/helm/cluster.go
Outdated
// CheckRadiusInstall checks if the Radius release is installed in the given kubeContext and returns an InstallState object | ||
// with the version of the release if installed, or an error if an error occurs while checking. | ||
func CheckRadiusInstall(kubeContext string) (InstallState, error) { | ||
// QueryRelease checks to see if a release is deployed to a namespace for a given kubecontext. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
// QueryRelease checks to see if a release is deployed to a namespace for a given kubecontext. | |
// queryRelease checks to see if a release is deployed to a namespace for a given kubecontext. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the 3 outputs of this function? Can we add a doc for that too?
pkg/cli/helm/cluster.go
Outdated
// CheckRadiusInstall checks if the Radius release is installed in the given kubeContext and returns an InstallState object | ||
// with the version of the release if installed, or an error if an error occurs while checking. | ||
func CheckRadiusInstall(kubeContext string) (InstallState, error) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra line can be removed
a3bbec4
to
2e640bb
Compare
After some discussion with @rynowak and a review of the current codebase where contour is uninstalled we agreed to be consistent and uninstall dapr |
Yes 👍. I was under the impression that we left Contour alone during uninstall. It turns out we remove it. @superbeeny and I made the executive decision to be consistent with our existing behavior in the absence of user feedback. Both options (leave-installed, uninstall) have downsides. If we get feedback we'll change this. I'll remind everyone that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me 👍 We should merge this now that we've branched for 0.39. We will be layering other changes on top for 0.40.
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
9b6aaa2
to
9bc827a
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
These workflows may need to be tested/updated since Dapr is installed via Helm in those. I see some functional tests of this PR fail at the Install Radius step: https://github.com/radius-project/radius/actions/runs/11865183744/job/33070281563?pr=8033. |
Signed-off-by: Nick Beenham <Nicholas_Beenham@cable.comcast.com>
Signed-off-by: Nick Beenham <Nicholas_Beenham@cable.comcast.com>
Signed-off-by: Nick Beenham <Nicholas_Beenham@cable.comcast.com>
Signed-off-by: Nick Beenham <Nicholas_Beenham@cable.comcast.com>
Signed-off-by: Nick Beenham <Nicholas_Beenham@cable.comcast.com>
9ec2ba0
to
2a1affb
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: Nick Beenham <Nicholas_Beenham@cable.comcast.com>
My first guess is that we're not detecting that dapr is already installed, should be easy enough to replicate ... thanks!! |
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: Nick Beenham <Nicholas_Beenham@cable.comcast.com>
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
# - name: Install dapr into cluster | ||
# run: | | ||
# wget -q https://raw.githubusercontent.com/dapr/cli/master/install/install.sh -O - | /bin/bash -s ${{ env.DAPR_VER }} | ||
# dapr init -k --wait --timeout 600 --runtime-version ${{ env.DAPR_VER }} --dashboard-version ${{ env.DAPR_DASHBOARD_VER }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @superbeeny - I have a question about removing this.
Does rad install kubernetes
install fail when Dapr is already installed? I think we should make sure that case succeeds.
I'm trying to assess whether this is being removed because it's unnecessary, or whether it's being removed because it's broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I'd prefer this be deleted rather than commented out. As well as the env-vars if they are unused. We have source control to see the history 👍
We can resolve that after the question above ^^^.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rad init will not fail for an existing dapr install
if err != nil { | ||
return false, err | ||
} | ||
|
||
err = ApplyContourHelmChart(options.Contour, kubeContext) | ||
// Install Dapr | ||
daprFound, err := ApplyHelmChart(clusterOptions.Dapr, kubeContext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per this question, we might need an "if !(Dapr is installed)" here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more question here: https://github.com/radius-project/radius/pull/8033/files#r1851230293
I think this is almost ready to go.
…bbreviations (radius-project#8073) # Description Updated cmd line functions to use `environment` in place of `env` and `initialize` in place of `init`. Both abbreviations are now aliased so this is a non breaking change. ## Type of change - This pull request fixes a bug in Radius and has an approved issue (issue link required). Fixes: radius-project#8043 Signed-off-by: Nick Beenham <Nicholas_Beenham@cable.comcast.com>
Signed-off-by: Nick Beenham <Nicholas_Beenham@cable.comcast.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- this is awesome! thanks @superbeeny
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Description
Installing the Dapr Helm Chart via the CLI
Type of change
Fixes: #8032
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: