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

Remove helm #625

Merged
merged 11 commits into from
Apr 20, 2021
Merged

Remove helm #625

merged 11 commits into from
Apr 20, 2021

Conversation

kalantar
Copy link
Member

@kalantar kalantar requested a review from sriumcp April 19, 2021 18:52
@@ -30,6 +30,7 @@ if [[ ! " ${NETWORK_LAYERS[@]} " =~ " ${1} " ]]; then
fi

# Step 1: Export correct tags for install artifacts
export GIT_ACCOUNT="${GIT_ACCOUNT:-iter8-tools}"
export TAG="${TAG:-v0.3.0}"
Copy link
Member

Choose a reason for hiding this comment

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

Tag needs to change here to v0.3.2

echo "Installing Iter8"
curl -s https://raw.githubusercontent.com/iter8-tools/iter8-install/main/install.sh | bash
echo "Installing Iter8 with Knative support"
curl -s https://raw.githubusercontent.com/${GIT_ACCOUNT}/iter8-install/${TAG}/install.sh | bash
Copy link
Member

Choose a reason for hiding this comment

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

${TAG} needs to be replaced with main

Copy link
Member Author

Choose a reason for hiding this comment

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

@sriumcp I don't understand why this is the case. Why do we not want to install a particular version that the sample was written/tested against?

Copy link
Member

@sriumcp sriumcp Apr 19, 2021

Choose a reason for hiding this comment

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

What we want to ensure is that a specific version of iter8 corresponds to a specific version of iter8-install (and iter8ctl).

Once you pin the version of iter8-install, by extension, you also pin the version of handler, etc3 and iter8-analytics.

If each tutorial points to a different version of iter8-install, then we will pretty soon be staring at an unmanageable mess in terms of dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Re: testing, we do want to test the samples against the iter8-install version that install.md points to.

If the tutorials break for the version installed by install.md, we have a problem, and shouldn't be merging in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok. I think I follow the expected behavior. I have undone most of the changes in this script.

echo "Installing Iter8's Prometheus add-on"
curl -s https://raw.githubusercontent.com/iter8-tools/iter8-install/main/install-prom-add-on.sh | bash
curl -s https://raw.githubusercontent.com/${GIT_ACCOUNT}/iter8-install/${TAG}/install-prom-add-on.sh | bash
Copy link
Member

Choose a reason for hiding this comment

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

${TAG} needs to change to main

# Step 6: Install Iter8's Prometheus add-on
# Step 6: Install sample metrics for Knative and Iter8's Prometheus add-on
echo "Installing sample metrics"
kubectl apply -f https://raw.githubusercontent.com/${GIT_ACCOUNT}/iter8-install/${TAG}/metrics/build.yaml
Copy link
Member

Choose a reason for hiding this comment

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

This ${TAG} is fine as it is.

@@ -26,7 +26,7 @@ curl -s https://raw.githubusercontent.com/iter8-tools/iter8-install/main/install
set -e
Copy link
Member

Choose a reason for hiding this comment

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

@kalantar If you agree, I am in favor of removing the ??? info "Look inside install.sh" tooltip altogether in install.md. It is more work for us in terms of ensuring consistency/correctness of the documentation

Copy link
Member Author

Choose a reason for hiding this comment

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

agree. done.

@@ -56,7 +56,7 @@ curl -s https://raw.githubusercontent.com/iter8-tools/iter8-install/main/install
set -e
Copy link
Member

Choose a reason for hiding this comment

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

@kalantar If you agree, I am in favor of removing the ??? info "Look inside instal-prom-add-on.sh" tooltip altogether in install.md. It is more work for us in terms of ensuring consistency/correctness of the documentation. Of course, folks can navigate to the GitHub page and look inside!

Copy link
Member Author

Choose a reason for hiding this comment

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

agree. done.

Copy link
Member

@sriumcp sriumcp left a comment

Choose a reason for hiding this comment

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

minor fixes

---
# Helm v3 uses secrets to store Helm release information.
#
# By default, when Helm is used within Iter8 experiments, secrets
Copy link
Member

Choose a reason for hiding this comment

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

This is not true any longer.

# Helm v3 uses secrets to store Helm release information.
#
# By default, when Helm is used within Iter8 experiments, secrets
# will be located in the Iter8 namespace.
Copy link
Member

Choose a reason for hiding this comment

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

No longer true.

# By default, when Helm is used within Iter8 experiments, secrets
# will be located in the Iter8 namespace.
#
# This role binding enables Iter8 handler to read
Copy link
Member

Choose a reason for hiding this comment

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

Default namespace

# will be located in the Iter8 namespace.
#
# This role binding enables Iter8 handler to write
# secrets in the namespace where Iter8 is installed
Copy link
Member

Choose a reason for hiding this comment

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

default namespace

@@ -98,7 +106,7 @@ sed "s+URL_VALUE+${URL_VALUE}+g" $ITER8/samples/knative/canaryprogressive/fortio
restartPolicy: Never
```

## 3. Create Iter8 experiment
## 4. Create Iter8 experiment
Copy link
Member

Choose a reason for hiding this comment

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

When you look inside the Iter8-experiment, it still says --namespace=iter8-system. The new experiment should be copied in here...

```shell
kubectl delete -f $ITER8/samples/knative/canaryprogressive/experiment.yaml
kubectl delete -f $ITER8/samples/knative/canaryprogressive/fortio.yaml
helm uninstall sample-app --namespace=iter8-system
helm uninstall sample-app --namespace=default
```

???+ info "Understanding what happened"
Copy link
Member

Choose a reason for hiding this comment

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

Understanding what happened also needs to change -- iter8-system must become default

Copy link
Member

@sriumcp sriumcp left a comment

Choose a reason for hiding this comment

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

lgtm

@sriumcp sriumcp merged commit 06c808e into iter8-tools:master Apr 20, 2021
@kalantar kalantar deleted the remove-helm branch May 4, 2021 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants