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

refactor: make all run options have shorthand vars #441

Conversation

DataDavD
Copy link
Contributor

@DataDavD DataDavD commented May 7, 2024

Refactor: Simple nitpick refactor to have all run option flags have shorthand vars.

Description

I was looking at my old PR that added hashes as run options flags and realized it and others did not have shorthand vars like the majority of the run options flags. So, I threw this nit PR up just in case y'all believe its best to align and have all run options flags have shorthand vars.

Feel free to close PR if you don't agree.

Copy link

netlify bot commented May 7, 2024

Deploy Preview for witness-project ready!

Name Link
🔨 Latest commit fcdc2f4
🔍 Latest deploy log https://app.netlify.com/sites/witness-project/deploys/666b6700d798200008518d4f
😎 Deploy Preview https://deploy-preview-441--witness-project.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@DataDavD DataDavD force-pushed the ddansby/refactor/NOISSUE/make-run-options-all-be-TypeVarP-flags branch from a23868a to 5e96483 Compare May 7, 2024 01:43
@DataDavD
Copy link
Contributor Author

DataDavD commented May 7, 2024

fyi forced pushed the rebase converting ts var to u (forgot to change that before pushing original 😅 ), so docgen step would need to be rerun.

options/run.go Outdated
cmd.Flags().BoolVar(&ro.Tracing, "trace", false, "Enable tracing for the command")
cmd.Flags().StringSliceVar(&ro.TimestampServers, "timestamp-servers", []string{}, "Timestamp Authority Servers to use when signing envelope")
cmd.Flags().BoolVarP(&ro.Tracing, "trace", "t", false, "Enable tracing for the command")
cmd.Flags().StringSliceVarP(&ro.TimestampServers, "timestamp-servers", "u", []string{}, "Timestamp Authority Servers to use when signing envelope")
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan with u as the short-hand for TSA but, t is the only thing that makes sense for tracing... 😅. @ChaosInTheCRD or @mikhailswift either of you have a good suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely agree @jkjell. I wasn't sure what else to use tbh, because as you noted t was already used aptly for tracing, as is s for StepName.

Maybe a for Authority?

Let me know what y'all think. I can also understand if y'all believe some of these actually should not have single shorthand letter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey y'all 👋 . Sorry to bother y'all, but any update on this?

Copy link
Member

Choose a reason for hiding this comment

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

For some reason, there's an issue with Cobra for these changes... 🤔

panic: unable to redefine 'h' shorthand in "run" flagset: it's already used for "hashes" flag

☝️ is in the e2e test and docs GHA workflows for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm let me take a look, sorry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh yea I think the issue is can't overwrite help shorthand -h. Doh

Copy link
Contributor Author

@DataDavD DataDavD Jun 3, 2024

Choose a reason for hiding this comment

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

with regards to timestamp servers and tracing, may be better with r and t respectively, but that's just my personal preference.

I'm starting to think, this PR can probably get trashed, probably overkill tbh and I got ahead of myself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @jkjell, did you see my previous comment. Do you think I should close this PR, or do you think it would still be beneficial to have some of the run options have a shorthand variable? I think we can scrap trying to force all run options to have a shorthand variable, but I think it would be beneficial to have hashes have a shorthand variable. What do you think? I'm also happy to close this PR and open an issue to discuss there if needed.

Copy link
Member

@jkjell jkjell Jun 10, 2024

Choose a reason for hiding this comment

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

Let's go with r and t. 👍 I thought about recommending d for hashes but, then I figured we'd get a lot of grief for not understanding the difference between hashing algorithms vs their resulting digest. 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @jkjell FYI just pushed my updates.

@jkjell jkjell force-pushed the ddansby/refactor/NOISSUE/make-run-options-all-be-TypeVarP-flags branch 2 times, most recently from ba87f5b to faa1678 Compare May 28, 2024 21:38
@jkjell jkjell force-pushed the ddansby/refactor/NOISSUE/make-run-options-all-be-TypeVarP-flags branch from faa1678 to a99af04 Compare June 6, 2024 18:24
@DataDavD DataDavD force-pushed the ddansby/refactor/NOISSUE/make-run-options-all-be-TypeVarP-flags branch from a99af04 to 7ce1ee0 Compare June 11, 2024 18:58
@jkjell jkjell force-pushed the ddansby/refactor/NOISSUE/make-run-options-all-be-TypeVarP-flags branch from 7ce1ee0 to e411dac Compare June 11, 2024 23:48
@DataDavD DataDavD force-pushed the ddansby/refactor/NOISSUE/make-run-options-all-be-TypeVarP-flags branch 3 times, most recently from 7854342 to 1b569e7 Compare June 13, 2024 03:06
Simple nitpick refactor to have all run option flags (except hashes
since there isn't really a good shorthand right now, given h is used for
help, and it would be a good idea to be explicit for≠ hashes) have
shorthand vars.

Signed-off-by: David Dansby <39511285+DataDavD@users.noreply.github.com>
@jkjell jkjell force-pushed the ddansby/refactor/NOISSUE/make-run-options-all-be-TypeVarP-flags branch from 1b569e7 to fcdc2f4 Compare June 13, 2024 21:39
@jkjell jkjell merged commit f07725e into in-toto:main Jun 13, 2024
17 checks passed
@DataDavD
Copy link
Contributor Author

@jkjell thank you!!!!

@DataDavD DataDavD deleted the ddansby/refactor/NOISSUE/make-run-options-all-be-TypeVarP-flags branch June 15, 2024 20:07
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