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

DATAGO-67636: Update README.md to include CLI commands plus some general cleanup #157

Merged
merged 15 commits into from
Feb 1, 2024

Conversation

gregmeldrum
Copy link
Collaborator

@gregmeldrum gregmeldrum commented Jan 31, 2024

What is the purpose of this change?

Update the README.md to reflect the new CLI commands.

How was this change implemented?

Added a new section to the README with an explanation of how to use it.

How was this change tested?

Reviewed by @AndreaKelso

Is there anything the reviewers should focus on/be aware of?

No


* Docker
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@AndreaKelso Here is the rewording for the prerequisites section.

@@ -180,10 +180,74 @@ curl -X 'GET' \
--output ih9z9lwwv5r.zip
```

# Manually uploading scans to Event Portal
## Running a Discovery Scan and File Export Using the CLI

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@AndreaKelso Added additional context for the CLI to make it clear the CLI can be accessed from the docker container or the jar file.

`run -d -v /path/to/file/AcmeRetailStandalone.yml:/config/ema.yml -v /path/to/scandir:/tmp --env CMD_LINE_ARGS=${CMD_LINE_ARGS} --env BROKER_PASSWORD=${BROKER_PASSWORD} --name event-management-agent event-management-agent:latest`

## Running a Discovery File Upload Using the CLI

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@AndreaKelso I was missing the word "upload" in the first sentence. Hopefully it's clear now.

@gregmeldrum gregmeldrum marked this pull request as ready for review January 31, 2024 21:07
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@moodiRealist moodiRealist left a comment

Choose a reason for hiding this comment

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

Added two minor comments, looks good otherwise!

```
CMD_LINE_ARGS="scan mysolacebroker /path/to/scan.zip --springdoc.api-docs.enabled=false --spring.main.web-application-type=none"
docker run -d -v /path/to/file/AcmeRetailStandalone.yml:/config/ema.yml -v /path/to/scandir:/tmp --env CMD_LINE_ARGS=${CMD_LINE_ARGS} --env BROKER_PASSWORD=${BROKER_PASSWORD} --name event-management-agent event-management-agent:latest
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed that there is copy to clipboard icon in git when you look at the file, and that copy will copy everything in the block. Therefore, I think it is better if we introduce 1 command per block to prevent any headache of copy pasting more than intended.
Screenshot 2024-02-01 at 11 54 01 AM

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gregmeldrum can you please use "```" instead? The new refactoring makes the code block read weird and doesn't have the copy to clipboard option anymore.
Screenshot 2024-02-01 at 1 16 28 PM

README.md Show resolved Hide resolved
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@gregmeldrum gregmeldrum merged commit 5dc025e into main Feb 1, 2024
6 checks passed
@gregmeldrum gregmeldrum deleted the greg/DATAGO-67636-readme branch February 6, 2024 15:19
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.

3 participants