Skip to content

Conversation

yelizhenden-mdb
Copy link
Collaborator

@yelizhenden-mdb yelizhenden-mdb commented Jan 16, 2025

Proposed changes

Jira ticket: CLOUDP-290414

This PR implements the data dump of the provided or default metric collection results file to S3 with given env variables.

The env variables will be provided by the GitHub Actions file, however, the script also allows a local .env file for local testing purposes if there are no env variables defined before code execution.

Testing

First data dump is made by running dataDump.js file with .env file filled with credentials

Checklist

  • I have signed the MongoDB CLA
  • I have added tests that prove my fix is effective or that my feature works

Changes to Spectral

  • I have read the README file for Spectral Updates

Further comments

Differences from the ticket description:

  • The code dumps the data in JSON format (not Parquet format) for testing purposes. It will be decided with DPE if Parquet format is needed
  • The code dumps the full data for now. It will be decided with DPE how we can proceed with the incremental data ingestion approach after discussing the nature of the data itself

@yelizhenden-mdb yelizhenden-mdb marked this pull request as ready for review January 16, 2025 17:42
@yelizhenden-mdb yelizhenden-mdb requested a review from a team as a code owner January 16, 2025 17:42
Comment on lines +33 to +37
credentials: {
accessKeyId: AWSConfig.aws.accessKeyId,
secretAccessKey: AWSConfig.aws.secretAccessKey,
},
region: AWSConfig.aws.region,
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should not use API keys anymore,
we've migrated other work to use assume role - example (https://github.com/mongodb/openapi/pull/329/files)

Copy link
Collaborator

Choose a reason for hiding this comment

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

if the owner team for the bucket can't provide this approach, I'm happy to approve if we have a ticket capturing this migration

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me check with them and update here accordingly 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I checked with them, and they confirmed they’ll add the trust policy on their end and share it with us. Would it be okay if I create a ticket to update the credentials in the code once we have role-to-assume ARN and update the GitHub secrets accordingly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

* Upload a file to an S3 bucket.
* @param filePath
*/
async function uploadMetricCollectionDataToS3(filePath = config.defaultMetricCollectionResultsFilePath) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[q] will we have tests added as a separate PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you asking if we will have integration tests to check whether the file is uploaded correctly to the S3 bucket?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now, we don’t have programmatic Read or Delete access, so we can’t verify the file once it’s written or delete it from the bucket. I’ll go ahead and create a ticket for future improvement so we can add these tests once we have the access.

@yelizhenden-mdb yelizhenden-mdb merged commit 51c1d3b into main Jan 20, 2025
13 checks passed
@yelizhenden-mdb yelizhenden-mdb deleted the CLOUDP-290414 branch January 20, 2025 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants