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.

Sign up for GitHub

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

Jump to bottom

Integration tests executed on a real deployment as part of the CICD - Redshift Shares #1643

Merged
merged 22 commits into from
Oct 22, 2024
Merged

Integration tests executed on a real deployment as part of the CICD - Redshift Shares #1643

merged 22 commits into from
Oct 22, 2024

Conversation

dlpzx
Copy link
Contributor

@dlpzx dlpzx commented Oct 16, 2024 •
edited
Loading

⚠️ MERGE AFTER #1636

Feature or Bugfix

  • Feature: Testing

Detail

Add integration tests for Redshift shares. Implements #1620

  • Implemented inside the shares modules in a subdirectory so that each share type can have its own conftest but still re-use common methods from shares queries
  • This PR is focused on testing the Redshift shares functionality, it does not include all tests that test the workflow of the share (e.g. submit, reject...)
  • It does not validate if after a share the user has access to data. We could implement it using the Redshift Data API, but I left it as optional for a separate PR

Tested

Locally:
image

Relates

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

  • Does this PR introduce or modify any input fields or queries - this includes
    fetching data from storage outside the application (e.g. a database, an S3 bucket)?
    • Is the input sanitized?
    • What precautions are you taking before deserializing the data you consume?
    • Is injection prevented by parametrizing queries?
    • Have you ensured no eval or similar functions are used?
  • Does this PR introduce any functionality or component that requires authorization?
    • How have you ensured it respects the existing AuthN/AuthZ mechanisms?
    • Are you logging failed auth attempts?
  • Are you using or adding any cryptographic features?
    • Do you use a standard proven implementations?
    • Are the used keys controlled by the customer? Where are they stored?
  • Are you introducing any new policies/roles/users?
    • Have you used the least-privilege principle? How?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

dlpzx added 17 commits October 9, 2024 09:55
@dlpzx
RS integration test - added queries and skeleton
d367e3e
@dlpzx
Added redshift-connection tests
d658890
@dlpzx
Added redshift-dataset tests
528bcc4
@dlpzx
Fix Nones in redshift cdk policy and add update stack on connection d…
6b64613 
…elete
@dlpzx
Merge branch 'feat/redshift-dataset-integration-test' into feat/redsh…
a55e0ce 
…ift-dataset-integration-test-2
@dlpzx
Added redshift-dataset tests
31ed83b
@dlpzx
Fix assertion in test_redshift_schema_tables + clean PR
eaa2d53
@dlpzx
Merge remote-tracking branch 'origin/feat/redshift-dataset-integratio…
293fc30 
…n-test' into feat/redshift-dataset-integration-test-2

# Conflicts:
#	tests_new/integration_tests/modules/redshift_datasets/global_conftest.py
@dlpzx
Redshift-dataset tests
1e853c4
@dlpzx
PR review comments
e879794
@dlpzx
Merge remote-tracking branch 'origin/main' into feat/redshift-dataset…
821baea 
…-integration-test
@dlpzx
PR review comments
b2a2c76
@dlpzx
Merge remote-tracking branch 'origin/feat/redshift-dataset-integratio…
693a67c 
…n-test' into feat/redshift-dataset-integration-test-2

# Conflicts:
#	tests_new/integration_tests/modules/redshift_datasets/global_conftest.py
@dlpzx
Merge remote-tracking branch 'origin/main' into feat/redshift-dataset…
4885590 
…-integration-test-2

# Conflicts:
#	tests_new/integration_tests/core/environment/global_conftest.py
#	tests_new/integration_tests/modules/redshift_datasets/dataset_queries.py
#	tests_new/integration_tests/modules/redshift_datasets/global_conftest.py
#	tests_new/integration_tests/modules/redshift_datasets/test_redshift_connection.py
@dlpzx
PR comments integration tests redshift datasets
b2fa3f5
@dlpzx
First skeleton redshift shares integration test and organize files
758a5f6 
First skeleton redshift shares integration test
@dlpzx
Revert Redshift validation changes - not needed
e3c31a1
@dlpzx dlpzx linked an issue Oct 16, 2024 that may be closed by this pull request
Integration tests executed on a real deployment as part of the CICD - Redshift Shares #1620
Closed
@dlpzx dlpzx force-pushed the feat/redshift-dataset-integration-test-shares branch 2 times, most recently from 5a85b20 to 49294ca Compare October 16, 2024 09:09
@dlpzx dlpzx force-pushed the feat/redshift-dataset-integration-test-shares branch from 49294ca to 50639de Compare October 16, 2024 14:19
@dlpzx dlpzx requested a review from petrkalos October 17, 2024 08:45
@dlpzx dlpzx force-pushed the feat/redshift-dataset-integration-test-shares branch from 02cb9cd to 7ac2f28 Compare October 17, 2024 08:47
dlpzx added 2 commits October 17, 2024 10:55
@dlpzx
Merge remote-tracking branch 'origin/main' into feat/redshift-dataset…
6f7f866 
…-integration-test-shares

# Conflicts:
#	tests_new/integration_tests/modules/redshift_datasets/dataset_queries.py
#	tests_new/integration_tests/modules/redshift_datasets/global_conftest.py
@dlpzx
Added reapply tests with new poller
ef54f22
@dlpzx dlpzx marked this pull request as ready for review October 17, 2024 12:32
petrkalos
petrkalos reviewed Oct 22, 2024
View reviewed changes
tests_new/integration_tests/modules/redshift_datasets/global_conftest.py Outdated
Copy link
Contributor

@petrkalos petrkalos Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

would you mind applying this commit to make the redshift tests optional? **I will drop it from my PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added! Yes, it makes more sense here. The pytest skip is ultra cool

petrkalos
petrkalos reviewed Oct 22, 2024
View reviewed changes
tests_new/integration_tests/modules/shares/redshift_datasets_shares/aws_clients.py Outdated
log.info(f'Datashare deauthorized successfully: {datashare_arn}')
return response
except ClientError as e:
log.error(f'Error deauthorizing datashare: {e}')
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: log.exception

petrkalos
petrkalos approved these changes Oct 22, 2024
View reviewed changes
Copy link
Contributor

@petrkalos petrkalos left a comment

Choose a reason for hiding this comment

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

lgtm, feel free to merge after incorporating the "optional" tests commit

@dlpzx dlpzx merged commit 3747c9d into main Oct 22, 2024
9 checks passed
@dlpzx dlpzx deleted the feat/redshift-dataset-integration-test-shares branch October 25, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrkalos petrkalos petrkalos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integration tests executed on a real deployment as part of the CICD - Redshift Shares
2 participants
@dlpzx @petrkalos