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.

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

SEAB-6360: download github event from s3 #491

Merged
merged 15 commits into from
May 24, 2024
Merged

Conversation

hyunnaye
Copy link
Contributor

@hyunnaye hyunnaye commented May 9, 2024

Description
This PR creates a module githubdelivery and adds a command download-event to return the github payload from s3 from its key.

Review Instructions

It was the easiest to run on CloudShell
Steps I used was

  1. Install Java sudo yum install java-17-amazon-corretto
  2. clone the repo
  3. Build only the githubdelivery module by running ./mvnw install -pl githubdelivery -am
  4. Configure your github-delivery.config for QA (template in templates folder)
  5. Go to the module and run java -jar target/githubdelivery-1.16.0-SNAPSHOT.jar download-event -k "2024-05-15/004701ac-1307-11ef-94b7-6b9aec991740" and verify the payload is printed on the console.

Issue
https://ucsc-cgl.atlassian.net/browse/SEAB-6360

Security
If there are any concerns that require extra attention from the security team, highlight them here.

Please make sure that you've checked the following before submitting your pull request. Thanks!

  • Check that you pass the basic style checks and unit tests by running mvn clean install in the project that you have modified (until https://ucsc-cgl.atlassian.net/browse/SEAB-5300 adds multi-module support properly)
  • Ensure that the PR targets the correct branch. Check the milestone or fix version of the ticket.
  • If you are changing dependencies, check with dependabot to ensure you are not introducing new high/critical vulnerabilities
  • If this PR is for a user-facing feature, create and link a documentation ticket for this feature (usually in the same milestone as the linked issue). Style points if you create a documentation PR directly and link that instead.

@hyunnaye hyunnaye self-assigned this May 9, 2024
Copy link

codecov bot commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.42%. Comparing base (98c476c) to head (07d957b).

Additional details and impacted files
@@            Coverage Diff             @@
##             develop     #491   +/-   ##
==========================================
  Coverage      48.42%   48.42%           
  Complexity       305      305           
==========================================
  Files             46       46           
  Lines           2480     2480           
  Branches         200      200           
==========================================
  Hits            1201     1201           
  Misses          1187     1187           
  Partials          92       92           
Flag Coverage Δ
metricsaggregator 39.63% <ø> (ø)
toolbackup 24.87% <ø> (ø)
tooltester 18.58% <ø> (ø)
topicgenerator 21.08% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hyunnaye hyunnaye changed the title test SEAB-6360: download github event from s3 May 21, 2024
@hyunnaye hyunnaye marked this pull request as ready for review May 21, 2024 20:46
@coverbeck
Copy link
Contributor

It was the easiest to run on CloudWatch.

CloudShell :) Too may terms with "Cloud" in them...

@kathy-t
Copy link
Contributor

kathy-t commented May 22, 2024

2. clone the repo

3. Build only the `githubdelivery` module by running `./mvnw install -pl githubdelivery -am`

You could perform a release once this is merged to develop so the reviewer can just wget the JAR from artifactory

githubdelivery/pom.xml Outdated Show resolved Hide resolved
githubdelivery/pom.xml Outdated Show resolved Hide resolved
githubdelivery/dependency-reduced-pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Commenting here but unrelated to this file: should also add this module to the CircleCI file so that it gets built

@Parameters(commandNames = { "download-event" }, commandDescription = "Download github event from S3 bucket using key.")
public static class DownloadEventCommand {

@Parameter(names = {"-k", "--key"}, description = "The key of the event in bucket. Format should be YYYY-MM-DD/deliveryid")
Copy link
Contributor

Choose a reason for hiding this comment

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

We can cross this bridge when we get there, but it could be useful to submit by date

Copy link
Member

Choose a reason for hiding this comment

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

Second, can create follow-up ticket to avoid this PR from dragging too long

Copy link
Member

@denis-yuen denis-yuen left a comment

Choose a reason for hiding this comment

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

Some clean-up items
Please link created tickets to the comments so we know what was done here and what remains to be done

githubdelivery/pom.xml Outdated Show resolved Hide resolved
githubdelivery/pom.xml Outdated Show resolved Hide resolved
githubdelivery/pom.xml Outdated Show resolved Hide resolved
githubdelivery/pom.xml Show resolved Hide resolved
githubdelivery/pom.xml Outdated Show resolved Hide resolved
githubdelivery/pom.xml Outdated Show resolved Hide resolved
githubdelivery/pom.xml Outdated Show resolved Hide resolved
@Parameters(commandNames = { "download-event" }, commandDescription = "Download github event from S3 bucket using key.")
public static class DownloadEventCommand {

@Parameter(names = {"-k", "--key"}, description = "The key of the event in bucket. Format should be YYYY-MM-DD/deliveryid")
Copy link
Member

Choose a reason for hiding this comment

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

Second, can create follow-up ticket to avoid this PR from dragging too long

final GithubDeliveryS3Client githubDeliveryS3Client = new GithubDeliveryS3Client(githubDeliveryConfig.getS3Config().bucket());

if ("download-event".equals(jCommander.getParsedCommand())) {
System.out.println(githubDeliveryS3Client.getGitHubDeliveryEventByKey(downloadEventCommand.getBucketKey()));
Copy link
Member

Choose a reason for hiding this comment

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

Should use logging framework, can be follow-up ticket

Comment on lines +38 to +60
public static void main(String[] args) throws IOException {
final GithubDeliveryCommandLineArgs commandLineArgs = new GithubDeliveryCommandLineArgs();
final JCommander jCommander = new JCommander(commandLineArgs);
final DownloadEventCommand downloadEventCommand = new DownloadEventCommand();
jCommander.addCommand(downloadEventCommand);

try {
jCommander.parse(args);
} catch (MissingCommandException e) {
jCommander.usage();
if (e.getUnknownCommand().isEmpty()) {
LOG.error("No command entered");
} else {
LOG.error("Unknown command");
}
exceptionMessage(e, "The command is missing", GENERIC_ERROR);
} catch (ParameterException e) {
jCommander.usage();
exceptionMessage(e, "Error parsing arguments", GENERIC_ERROR);
}

if (jCommander.getParsedCommand() == null || commandLineArgs.isHelp()) {
jCommander.usage();

Choose a reason for hiding this comment

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

This code section, which creates the JCommander object and handles errors and prints usage information, appears to repeat in some of our other utilities. If, upon inspection, we find that it's repeated more-or-less verbatim, we might consider moving it to a helper (at some time, doesn't need to happen now). Then, we could do something like:

    final GithubDeliveryCommandLineArgs commandLineArgs = new GithubDeliveryCommandLineArgs();
    final DownloadEventCommand downloadEventCommand = new DownloadEventCommand();
    final JCommander jCommander = JCommanderHelper.createAndParse(commandLineArgs, downloadEventCommand);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add this to the followup :)

githubdelivery/src/main/resources/logback.xml Outdated Show resolved Hide resolved
@hyunnaye
Copy link
Contributor Author

I created ticket https://ucsc-cgl.atlassian.net/browse/SEAB-6452 For followup

@hyunnaye hyunnaye requested a review from kathy-t May 22, 2024 20:05
Copy link
Member

@denis-yuen denis-yuen left a comment

Choose a reason for hiding this comment

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

minor redundancy

githubdelivery/pom.xml Outdated Show resolved Hide resolved
githubdelivery/pom.xml Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented May 23, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
3.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@hyunnaye hyunnaye merged commit 081311e into develop May 24, 2024
10 of 11 checks passed
@hyunnaye hyunnaye deleted the feature/SEAB-6360 branch May 24, 2024 14:04
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.

5 participants