-
Notifications
You must be signed in to change notification settings - Fork 27
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
Setup Initial Jenkins Git Structure #744
Setup Initial Jenkins Git Structure #744
Conversation
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #744 +/- ##
=============================================
- Coverage 89.37% 68.21% -21.17%
- Complexity 0 1578 +1578
=============================================
Files 46 270 +224
Lines 2702 11139 +8437
Branches 0 736 +736
=============================================
+ Hits 2415 7598 +5183
- Misses 287 3138 +2851
- Partials 0 403 +403
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
access_result['connection_message'] = "Successfully connected!" | ||
access_result['cluster_version'] = response_json['version']['number'] | ||
access_result['connection_established'] = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be nice to make this access_result something like a named tuple or a very simple class so we can make a type guarantee on the return and the caller knows exactly what to assume they can get
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea have made a simple class around this
|
||
|
||
# As a default we exclude system indices and searchguard indices | ||
def clear_indices(cluster: Cluster): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used anywhere? It's definitely very helpful for our testing, but I think we should put it behind a verification step where the user explicitly acknowledges that they're going to delete all the data in their cluster.
(helper for that https://click.palletsprojects.com/en/8.1.x/prompts/#confirmation-prompts)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
potentially even do something like only create a CLI command for it (or at least only enable it) if there's an env variable like DEBUG_MODE
or I_WANT_TO_DO_DANGEROUS_THINGS_TO_MY_CLUSTER
set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went ahead and added this as a CLI option, with a confirmation step added to it, thanks for the link
raise NotImplementedError(f"Auth type {self.auth_type} is not currently support for executing " | ||
f"benchmark workloads") | ||
logger.info(f"Running opensearch-benchmark with '{workload}' workload") | ||
subprocess.run(f"opensearch-benchmark execute-test --distribution-version=1.0.0 " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the benchmark outputs print to stdout or are they captured here? Kind of nice to show the user that something's happening, so I think I'm moderately in favor of not-capturing, but don't feel super strongly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now they are printing to stdout which I kinda like as well. I'm open to change that though if we decide we don't like it
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
# Conflicts: # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/cluster.py
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
click.echo("Performing clear indices operation...") | ||
click.echo(logic_clusters.clear_indices(cluster_focus)) | ||
else: | ||
if click.confirm('Clearing indices can result in data loss, are you sure you want to continue?'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we phrase this slightly more strongly: "Clearing indices WILL result in the loss of all data on the {source/target} cluster. Are you sure you want to continue?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to this wording
"""[Caution] Clear indices on a source or target cluster""" | ||
cluster_focus = ctx.env.source_cluster if cluster.lower() == 'source' else ctx.env.target_cluster | ||
if acknowledge_risk: | ||
click.echo("Performing clear indices operation...") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor, but I'd also reiterate in these messages which cluster is being cleared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, added
assert result.exit_code == 0 | ||
|
||
|
||
def test_cli_cluster_clear_indices(runner, env, mocker): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you do a version of this test without --acknowledge-risk
to make sure that it doesn't call clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 👍
//deployStep: { | ||
// echo 'Custom Test Step' | ||
//} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to use it as a reference that steps could be replaced if needed
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Description
This change introduces a shared library structure for our Jenkins pipelines. As can be seen, this allows creating a pipeline template that our actual pipeline files can utilize and configure as needed.
Along with this, this change includes some common functions that will be needed for testing and have been added to the console library, with a future change expected to move our python testing files to be able to use this console library as well.
Issues Resolved
Partially resolves: https://opensearch.atlassian.net/browse/MIGRATIONS-1753
Testing
Local/Cloud testing
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.