Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

Make bookstore app ports configurable in automated demo #2755

Closed
ksubrmnn opened this issue Mar 5, 2021 · 8 comments
Closed

Make bookstore app ports configurable in automated demo #2755

ksubrmnn opened this issue Mar 5, 2021 · 8 comments
Labels
area/demo Demo related area/tests Related to tests
Milestone

Comments

@ksubrmnn
Copy link
Contributor

ksubrmnn commented Mar 5, 2021

Make demo app ports configurable. This will help users customize the demos to suit their environments.

@addozhang
Copy link
Contributor

+1, 80 is not acceptable in openshift.

@ksubrmnn ksubrmnn changed the title Make bookstore apps configurable in automated demo Make bookstore app ports configurable in automated demo Mar 12, 2021
@draychev draychev added this to the v0.10.0 milestone Jun 2, 2021
@allenlsy
Copy link
Contributor

Are we talking about configuring the port forwarding port in auto demo? I think they are configurable now.

E.g. https://github.com/openservicemesh/osm/blob/main/scripts/port-forward-bookstore-ui.sh#L18

Or do I misunderstand the issue?

@ksubrmnn
Copy link
Contributor Author

ksubrmnn commented Jun 17, 2021

This issue is for this port (for all the demo apps):

@draychev
Copy link
Contributor

This GitHub Issue would also provide great value in instrumenting end-to-end tests where we can randomize app ports (within some ranges).
This will ensure that we are not accidentally hard-coding ports in Envoy configs etc.

@draychev draychev added the area/tests Related to tests label Jun 22, 2021
@shashankram
Copy link
Member

This GitHub Issue would also provide great value in instrumenting end-to-end tests where we can randomize app ports (within some ranges).
This will ensure that we are not accidentally hard-coding ports in Envoy configs etc.

The bookstore apps are not used in e2e tests, we use a combination of curl and httpbin. I agree some sort of fuzzing around the values used in e2e tests is valuable, but I don't see a benefit to complicating the existing bookstore demo apps to perform this sort of fuzz testing. Moreover, the automated demo (run-osm-demo.sh) is already getting complicated with the number of permutations being introduced.

@draychev Do you think this is still necessary?

@draychev
Copy link
Contributor

@shashankram This does not seem too critical.
@allenlsy If you already have a PR for this change (I understand this to be the case) - I would not be opposed to merging it.

@allenlsy
Copy link
Contributor

PR reopened. PTAL.

#3619

@allenlsy
Copy link
Contributor

Feature is not needed any more as we want to keep our demo application simple.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/demo Demo related area/tests Related to tests
Projects
None yet
Development

No branches or pull requests

5 participants