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

feat: Support for all input formats when running jake ddt or jake iq #125

Merged
merged 9 commits into from
Dec 2, 2022
Merged

feat: Support for all input formats when running jake ddt or jake iq #125

merged 9 commits into from
Dec 2, 2022

Conversation

sanzoghenzo
Copy link
Contributor

This pull request makes the following changes:

  • refactor SbomCommand._get_parser() method into a function in its own parser_selector module
  • move the two needed argparse arguments to add_parser_selector_arguments function in the same module
  • use those function in both ddt and iq commands.

BREAKING CHANGE: changed iq -i switch to -a, removed sbom -t switch (only -it works)

It relates to the following issue #s:

cc @bhamail / @DarthHater

closes #104

BREAKING CHANGE: changed iq -i switch to -a, removed sbom -t switch (only -it works)
@sonatypecla
Copy link

sonatypecla bot commented Oct 28, 2022

Thanks for the contribution! Unfortunately we can't verify if the committer(s), Andrea Ghensi aghensi@systra.com, signed the CLA because they have not associated their commits with their GitHub user. Please follow these instructions to associate your commits with your GitHub user. Then sign the Sonatype Contributor License Agreement and this Pull Request will be revalidated.

@sanzoghenzo
Copy link
Contributor Author

I'm having trouble verifying my new corporate email address (the email filter is too strong, I suppose), I opened a ticket with my IT support to try to solve it.

In the meantime, I saw that the static_code_analysis-310-locked fails on circleci but runs fine on my machine...
I had poetry 1.1.15, but it works also if I downgrade to 1.1.11.

@sanzoghenzo
Copy link
Contributor Author

Email verified, still don't know how to solve the circleci error! please advise

@sanzoghenzo
Copy link
Contributor Author

Is there anyone out there? 😉

Copy link
Contributor

@bhamail bhamail left a comment

Choose a reason for hiding this comment

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

Still reviewing why my local CI build fails with vulnerabilities (maybe this is why the CI failed on the PR). Some questions regarding arg changes in line.

Aha, I figured out why the vulnerability failure was happening (changes that only exist on my machine). Doh!

I don't see why the CI build is failing in py39. I may push a change to update the poetry version (a shot in the dark to see if it helps). Happy to revert if it is no help.

Thanks for the PR, and apologies for the slow replies.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@sanzoghenzo
Copy link
Contributor Author

Thanks for the PR, and apologies for the slow replies.

Don't worry, as you can see I also struggle finding time to contribute to FOSS!

I hope I addressed everything in the last two commits!

Copy link
Contributor

@bhamail bhamail left a comment

Choose a reason for hiding this comment

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

Thanks again for you efforts on this! Some comments in line.

README.md Outdated
@@ -303,26 +318,41 @@ Access Sonatype's proprietary vulnerability data using `jake`:
```
> jake iq --help

usage: jake iq [-h] -s https://localhost:8070 -i APP_ID -u USER_ID -p PASSWORD [-t STAGE]
usage: jake iq [-h] -s https://localhost:8070 -i APP_ID -u USER_ID -p PASSWORD [-st STAGE]
Copy link
Contributor

Choose a reason for hiding this comment

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

So....don't shoot me for only thinking of this now, but another thought occurs: Would it be better (to avoid breaking changes) to make the new FILE_PATH arg be -f? (I would have no preference as to whether to keep --input or use something like --file-input for the long argument).

I mention this because while reviewing, I noticed the docs missed updating the -i to -a on line 321.

Also, I usually try to run the jake iq --help command in a terminal, and copy/paste the output into these docs. That helps ensure the docs/help are current too.

Question: Have you verified if the "old" iq arguments cause an exception? I'd rather we fail loudly (than silently) due to a breaking change. For example, if -i appId ... -t stage fails loudly when run with the newest changes?

Copy link
Contributor Author

@sanzoghenzo sanzoghenzo Nov 29, 2022

Choose a reason for hiding this comment

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

So is it ok to also change sbom's -i option to -f? I thought that I had to always use the -i flag for uniformity (and to DRY the code), but I just started using jake and don't really know what are the most used flags that are worth keeping.

You're right I should have used the --help flag to update the docs, I was being too lazy!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I went for the -f route for every command, and reverted what I could to before. -st has to stay to avoid conflicts with --type shorthand.
iq fails if the -t flag is used for stage since it only accepts the supported file formats.
Now the docs are in sync with the code!

Copy link
Contributor

@bhamail bhamail left a comment

Choose a reason for hiding this comment

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

Some minor doco tweaks (assuming I'm not doing something silly). Looks good to me!

docs/usage.rst Show resolved Hide resolved
docs/usage.rst Show resolved Hide resolved
docs/usage.rst Show resolved Hide resolved
@bhamail bhamail merged commit 9a597b5 into sonatype-nexus-community:main Dec 2, 2022
@sanzoghenzo sanzoghenzo deleted the feat/multiparser branch December 3, 2022 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Support for all input formats when running jake ddt or jake iq
2 participants