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

Fix star wars demo #5300

Merged
merged 2 commits into from Aug 24, 2018
Merged

Fix star wars demo #5300

merged 2 commits into from Aug 24, 2018

Conversation

aanm
Copy link
Member

@aanm aanm commented Aug 22, 2018

Summary of changes:

This reverts commit c162b90 as the purpose of the test to point to master is to break the CI so we are aware the demo was changed and the test needs to be re-written.


This change is Reviewable

@aanm aanm requested a review from a team as a code owner August 22, 2018 13:54
@aanm aanm added the area/CI Continuous Integration testing issue or flake label Aug 22, 2018
@aanm
Copy link
Member Author

aanm commented Aug 22, 2018

test-me-please

Copy link
Member

@ianvernon ianvernon left a comment

Choose a reason for hiding this comment

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

I highly disagree with the sentiment of this PR. If you look at the commit which broke the test, all it did was move files around: cilium/star-wars-demo@28ac07e

We should not have links to 'pointers' to commits in our CI. This is similar to not using 'latest' for an image tag. What if, say, a bad commit is pushed to master of the star-wars-demo repo and we are trying to do critical backports to v1.0, v1.1, and v1.2? It will break the CI, which is exactly what happened last week. This resulted in a bunch of failed builds and created a bunch of noise. I'd be alright with updating the commit from the cilium/star-wars-demo repository used in this test to the most recent one.

@aanm
Copy link
Member Author

aanm commented Aug 22, 2018

This resulted in a bunch of failed builds and created a bunch of noise.

And I'm glad it did, it make us to be aware of the problem and fix it quickly with your commit, otherwise it would be unnoticed, we would be making changes in Cilium and potentially break demos without realizing.

@ianvernon
Copy link
Member

I still (and will) disagree, because I am firmly of the belief that we should not be breaking CI based off of the state of other branches. The reason this test was added was to ensure we have coverage of the demo immediately in the CI- the easiest way of doing this was to port the demo itself to the CI, as it was marked as high priority at the time.

Instead of relying upon the state of that repository, let's distill what functionality of Cilium this is testing (GRPC with L4 and L7 policy) and add a test which tests said functionality that is located in the cilium/cilium repository. If we want to test the demo itself, we should add a test which runs in the cilium/star-wars-demo repository, because that's where the actual demo code is.

@tgraf
Copy link
Member

tgraf commented Aug 22, 2018

I think both arguments are valid. Both approaches verify a different requirement.

The original purpose of the test was to ensure that we are not in a position of doing a live demo based on the contents of that repo and be surprised. Which is what happened once and why Andre is bringing this up the way he is.

We have ample coverage of the functionality in other tests so if we follow Ian's understanding, then we could also remove the test entirely after a quick audit.

My proposal:
We keep the test and we refer to v1.0 and we change the README in the starwars-demo repo to clearly state that the v1.0 branch is what is being verified in the CI and that master is untested. Any changes to master need to be tagged and the tag in the CIlium needs to be updated.

@eloycoto
Copy link
Member

Hi,

I have a few things to mention here:

  • If we test, we should use master, as André did. If not the test is not relevant at all. Saying that, I think that we can create a custom test based on our ginkgo framework in that folder, and run when something changes and daily.
  • On the other hand, I need to say that this test has been useful in the last months, all the issues that we found about the endpoint regeneration was in this test or kafka, and mainly because it deploys more pods at the same time.

I'm happy if we do the following:

  • Write the test in that project, and run daily in Jenkins, and running with Nightly image for example.
  • Move this test as it is, with the commit and run that in Nightly.

Regards.

@tgraf
Copy link
Member

tgraf commented Aug 23, 2018

Let's avoid over-engineering this. We have a simple goal: We want to ensure that the demo always works. The version to demo does not have to be master. If we release it properly and declare it as such in the README then that is fine as well.

@ianvernon
Copy link
Member

The version to demo does not have to be master. If we release it properly and declare it as such in the README then that is fine as well.

+1

@aanm
Copy link
Member Author

aanm commented Aug 24, 2018

test-me-please

@tgraf tgraf requested review from a team August 24, 2018 08:55
@tgraf tgraf requested review from a team as code owners August 24, 2018 08:55
@tgraf tgraf requested a review from a team August 24, 2018 08:55
@tgraf tgraf requested a review from a team as a code owner August 24, 2018 08:55
@tgraf tgraf requested a review from a team August 24, 2018 08:55
daemon/policy.go Outdated
@@ -40,7 +39,7 @@ import (
"github.com/op/go-logging"
)

// TriggerPolicyUpdates triggers policy updates for every daemon's endpoint.
// RegenerateAllEndpoints triggers policy updates for every daemon's endpoint.

Choose a reason for hiding this comment

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

comment on exported method Daemon.TriggerPolicyUpdates should be of the form "TriggerPolicyUpdates ..."

@aanm
Copy link
Member Author

aanm commented Aug 24, 2018

test-me-please

@aanm aanm requested a review from ianvernon August 24, 2018 10:56
@aanm aanm merged commit 50f29e9 into master Aug 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration testing issue or flake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants