pants-plugins/uses_services: add support for uses=st2cluster in BUILD metadata #6279
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cognifloyd
requested review from
winem,
nzlosh,
rush-skills,
guzzijones,
amanda11 and
a team
pull-request-size
bot
added
the
size/L
amanda11
reviewed
Nov 18, 2024
amanda11
reviewed
Nov 18, 2024
amanda11
reviewed
Nov 18, 2024
amanda11
reviewed
Nov 18, 2024
amanda11
approved these changes
Nov 18, 2024
There was a problem hiding this comment.
@cognifloyd I've approved but left some comments. Most are about the copyright, which may well be correct, if those changes were started in 2023. But if not then please amend to 2024.
Doh! I messed up the copyright again. Thanks for catching that, I'll fix it today. |
The copyrights were copy/pasta. These files were written in 2024, not 2023.
amanda11
approved these changes
Nov 18, 2024
|
@cognifloyd lint checks failed |
…hosts/ports In the uses_services plugin, I am now adding env var support (in several PRs), but so far I've avoiding parsing conf files. Plus, allowing reconfiguration of the st2cluster hosts/ports would require the st2client and st2 dev cluster config to align. Today, the st2cluster integration tests hardcode st2client's base_url to `http://127.0.0.1`. Technically someone could override the endpoint URLs with ST2_{AUTH,API,STREAM}_URL env vars, but they would also need to make sure whatever cluster they use has the examples pack and other things handled by `launchdev.sh`. Today, the other alternative for using a different cluster would be to forward the service ports to the localhost at the standard ports. For now, just use a separate host for each endpoint so it is easier to support overriding via env vars and/or conf files at some point in the future--if someone needs that. Until then, we'll go with the minimal implementation that assumes the default hosts and ports.
cognifloyd
force-pushed
the
pants-plugins-uses_services-st2cluster
branch
from
491a4a2 to
b0767b9
Compare
|
K. I fixed my lint issue. All CI is passing 😄. One more review, and this can be merged. |
guzzijones
approved these changes
Nov 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR's commits were extracted from #6273 where I'm working on getting pants+pytest to run integration tests.
Today, we run a dev st2 cluster using
tools/launchdev.shfor all integration tests. However, only a subset of the tests actually make use of the running service. The only tests that require the dev st2 cluster are orquesta runner integration tests (which run today as part of integration tests) and the orquesta integration tests (inst2tests/integration/orquesta) that today run in a separate Orquesta Tests CI job.The rest of the integration tests actually start the
st2apior other process as part of the test, so they ignore the dev cluster, meaning CI doesn't need to the overhead of running the full cluster to run those tests. Plus, when running the tests locally, you don't need to run the dev cluster either, unless you need to run the orquesta tests.So, this PR adds a new
st2clusterservice topants-plugins/uses_servicesso that we can check for the running dev cluster and raise an error if the cluster isn't running when attempting to run the tests that need it.I also added an
st2clustertag to the BUILD metadata of these tests so that it is simple to run only the tests that needst2clusteror run allintegrationtagged tests except for thest2clustertagged tasks. To skip thest2clustertagged tasks, you might run pants like this:pants --tag=integration --tag=-st2cluster test ::.It might be helpful to review each commit of this PR. Sorry for the size of the first commit; I don't see a sane way to split this PR up any farther.
For background on
pants-plugins/uses_servicessee:pants-plugins/uses_servicesto check before running tests for required services (like mongo) #5864pants-plugins/uses_servicesto support checking for rabbitmq #5884pants-plugins/uses_servicesto support checking for redis #5893pants-plugins/uses_servicesimprovements #5898system_userdetection topants-plugins/uses_services(+mongodetection improvements) #6244