Fix 556-new branch

[macel94]

Description

Please explain the changes you've made

Issue reference

We strive to have all PRs 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:

• The quickstart code compiles correctly
• You've tested new builds of the quickstart if you changed quickstart code
• You've updated the quickstart's README if necessary
• If you have changed the steps for a quickstart be sure that you have updated the automated validation accordingly. All of our quickstarts have annotations that allow them to be executed automatically as code. For more information see mechanical-markdown. For user guide with examples see Examples.

[macel94]

@greenie-msft here is the new pr, can you please re-shchedule these tests? Thank you!

[macel94]

Linked to #556 and #763

[macel94]

@greenie-msft @paulyuk sorry for spamming you but without your help, being a first-time contributor, i can only wait.

thank you for your help and i hope in the future i will be faster in my contributions :)

[macel94]

@greenie-msft @msfussell @paulyuk

Hi, it doesn't seem to be using a new image containing the changes i made in the quickstarts, but the old one(i didn't understand we would follow this particular flow, so i studied now how things should work).

This i believe is because in the test pipeline it's using the latest image pushed in your current dapr github container repository:
https://github.com/dapr/quickstarts/blob/feature/556-tutorials-fix/tutorials/hello-kubernetes/deploy/node.yaml#L40
--> so these ones https://github.com/orgs/dapr/packages?repo_name=quickstarts

I see that you changed the workflow in this branch: 85d6297

But the problem is that this PR needs to be merged(tests here will never complete correctly).
After the merge of this PR, the build pipeline will run on this branch and only then, only until someone else pushes other changes on one of the others branches the trigger is listening to, tests will pass.

So i think we need to coordinate a bit to make this work :)

To recap the necessary steps:

1. until this PR(Fix 556-new branch #771, the current one) is merged, tests will not pass.
2. after the merge, i expect the build.yml pipeline to automatically start and publish the new images thanks to your workflow change on the trigger.
3. if when that pipeline ends publishing the new images, someone on your side(i don't have the necessary permissions) re-runs the tests from the old PR(Fix 556 #763) tests will use the new image and pass.

thank you for your help

P.S. sorry for all the noise, but don't you think this may seem a bit too complex to new contributors?
let me know if you're open to discuss on how to slim this process down or if you already know your desired flow, i'd be happy to help.

[macel94]

@greenie-msft @msfussell @paulyuk @yaron2
up 🚀

[paulyuk]

I see a simplification in the tutorial output and what we're matching, which I believe should help.

[paulyuk]

@greenie-msft @msfussell @paulyuk

Hi, it doesn't seem to be using a new image containing the changes i made in the quickstarts, but the old one(i didn't understand we would follow this particular flow, so i studied now how things should work).

This i believe is because in the test pipeline it's using the latest image pushed in your current dapr github container repository: https://github.com/dapr/quickstarts/blob/feature/556-tutorials-fix/tutorials/hello-kubernetes/deploy/node.yaml#L40 --> so these ones https://github.com/orgs/dapr/packages?repo_name=quickstarts

I see that you changed the workflow in this branch: 85d6297

But the problem is that this PR needs to be merged(tests here will never complete correctly). After the merge of this PR, the build pipeline will run on this branch and only then, only until someone else pushes other changes on one of the others branches the trigger is listening to, tests will pass.

So i think we need to coordinate a bit to make this work :)

To recap the necessary steps:

1. until this PR(Fix 556-new branch #771, the current one) is merged, tests will not pass.
2. after the merge, i expect the build.yml pipeline to automatically start and publish the new images thanks to your workflow change on the trigger.
3. if when that pipeline ends publishing the new images, someone on your side(i don't have the necessary permissions) re-runs the tests from the old PR(Fix 556 #763) tests will use the new image and pass.

thank you for your help

P.S. sorry for all the noise, but don't you think this may seem a bit too complex to new contributors? let me know if you're open to discuss on how to slim this process down or if you already know your desired flow, i'd be happy to help.
Yes sorry for lapse here over holidays.

The workflow is tricky when you need to change a test workflow or a container image, and effectively we need to merge and test in prod.

I'm very open to discussing how we can simplify this.

[macel94]

Hi @greenie-msft @msfussell @paulyuk

let me know if you need anything else but i think you're ready to merge this if it's ok with you and then re-trigger tests on the other PR to effectively see that they will pass.

THE DCO is not passing now because of someone else's missing sign, I think I'm not able to correct this(the commit was already in Master, I just synced from master in my fork/branch): 22ef222

Yes sorry for lapse here over holidays.

The workflow is tricky when you need to change a test workflow or a container image, and effectively we need to merge and test in prod.

I'm very open to discussing how we can simplify this.

For this matter I can create another Issue if you're ok with that, but the general workflow I'd expect would be:

As a contributor wannabe, I fork, make my changes, I am able to launch the build that publishes containers(not on dockerhub, just here on my GHCR that is where the demo takes the images from).

Something like what i did here: https://github.com/MACEL94/AKS-3-Tier-App/blob/master/.github/workflows/api.yaml#L24 but using

Then the tests should be run when i PR in my fork from my branch and target master(using my ghcr).

Then i can do a PR from my fork master to your dapr/quickstarts/master, this PR tests won't pass but at least you'll be able to request me and see in my public history and PR that with the new images, the PR would succeed.

If you took the time to truly read this, thank you for your time

[paulyuk]

Hi @greenie-msft @msfussell @paulyuk

let me know if you need anything else but i think you're ready to merge this if it's ok with you and then re-trigger tests on the other PR to effectively see that they will pass.

THE DCO is not passing now because of someone else's missing sign, I think I'm not able to correct this(the commit was already in Master, I just synced from master in my fork/branch): 22ef222

Yes sorry for lapse here over holidays.
The workflow is tricky when you need to change a test workflow or a container image, and effectively we need to merge and test in prod.
I'm very open to discussing how we can simplify this.

For this matter I can create another Issue if you're ok with that, but the general workflow I'd expect would be:

As a contributor wannabe, I fork, make my changes, I am able to launch the build that publishes containers(not on dockerhub, just here on my GHCR that is where the demo takes the images from).

Something like what i did here: https://github.com/MACEL94/AKS-3-Tier-App/blob/master/.github/workflows/api.yaml#L24 but using

Then the tests should be run when i PR in my fork from my branch and target master(using my ghcr).

Then i can do a PR from my fork master to your dapr/quickstarts/master, this PR tests won't pass but at least you'll be able to request me and see in my public history and PR that with the new images, the PR would succeed.

If you took the time to truly read this, thank you for your time

Yes I read it all and I understand. I would like our branches to be more isolated and allow end to end testing. This is a good idea but let's separate that from this fix to unblock the test in its current form.

Is the PR ready? If you run make validate and test locally things look good? I'm doing same now.

Also I sent an invite on Discord. Could you please accept if you'd like to coordinate a bit more?

[paulyuk]

We synced and goal is to merge this tomorrow Italy time after one more check.

[macel94]

@paulyuk
I pinged you on discord and sent you the screens of my successful tests on minikube in the devcontainer using the new code.

I can also post them here if you'd like me to.

[macel94]

@paulyuk @yaron2 @ItalyPaleAle

Recap of tests done in my local devcontainer

Before tests

minikube delete
minikube start

cd to tutorials/hello-kubernetes

helm repo add bitnami https://charts.bitnami.com/bitnami
helm repo update
helm install redis bitnami/redis
dapr init -k --wait

Tests not passing with the old container image

make validate

and the tests of course were not passing:

Generating the new image

Then I built the dockerfile with the node app locally(so my code was there) and i published it to dockerhub.

I changed the node.yaml in the deploy folder to reference that new local image(which will be the same that will be published by your ci once we merge).

Tests passing using the new image

Now that the image is online and public, i rerun make validate and finally they are green!

So yes, when you have time please i think you can merge the second PR, republish the images, re-run the tests in the original PR and then just merge that original one to master.