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

CLI specification #541

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

CLI specification #541

wants to merge 3 commits into from

Conversation

clairernovotny
Copy link
Member

@clairernovotny clairernovotny commented Jan 5, 2023

Adds CLI documentation for both Azure-Key-Vault and Certificate-Store commands

@clairernovotny clairernovotny requested a review from a team as a code owner January 5, 2023 18:48
Copy link

@erdembayar erdembayar left a comment

Choose a reason for hiding this comment

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

Thank you for spec, looks good. 👏
Left few comments.

docs/cli-spec.md Show resolved Hide resolved
docs/cli-spec.md Show resolved Hide resolved
docs/cli-spec.md Show resolved Hide resolved
docs/cli-spec.md Show resolved Hide resolved
docs/cli-spec.md Show resolved Hide resolved
- `-kvs | --azure-key-vault-client-secret` - The Client Secret to authenticate to the Azure Key Vault.
- `-kvc | --azure-key-vault-certificate` - The name of the certificate in Azure Key Vault.
- `-kvm | --azure-key-vault-managed-identity` - Use a Managed Identity to access Azure Key Vault.

Choose a reason for hiding this comment

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

Do we need [-v|--verbosity <LEVEL>] option here? Maybe customer want to see more details if there is issue with signing process.

Choose a reason for hiding this comment

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

Would be good to know what the options are here for this value to provide as generally the tool is run on the CI, so getting help from the tool directly involves more steps (and it's not as discernable from the code as it's a .NET enum value)

- `-f | --force` - Overwrites a signature if it exists.
- `-m | --max-concurrency` - Maximum concurrency (default is 4)
- `-fl | --filelist` - Path to file containing paths of files to sign within an archive
- `-fd | --file-digest` - The digest algorithm to hash the file with.

Choose a reason for hiding this comment

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

I expected it would be enum, right? Which algorithm does it default to?

Suggested change
- `-fd | --file-digest` - The digest algorithm to hash the file with.
- `-fd | --file-digest` <HASHALGORITHM> - The digest algorithm to hash the file with.

- `-tr | --timestamp-rfc3161` - Specifies the RFC 3161 timestamp server's URL.
- `-d | --description` - Description
- `-u | --descriptionUrl` - Description Url
- `-td | --timestamp-digest` - Used with the -tr switch to request a digest algorithm used by the RFC 3161 timestamp server.
Copy link

@erdembayar erdembayar Jan 6, 2023

Choose a reason for hiding this comment

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

This one is also expects enum value, right? If yes what are the expected values and default value?

```
**/ProjectAddIn1.*
**/setup.exe
```

Choose a reason for hiding this comment

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

Could we have sample section like dotnet nuget sign?

The parameters to the signing client are as follows:

`sign code AzureKeyVault [options] <files>`

Choose a reason for hiding this comment

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

Could we have Synopsis section like dotnet nuget sign?

docs/cli-spec.md Outdated

Signing an archive type (`.zip`, `.nupkg`, `.vsix`) will open up the archive and sign any supported file types. It is strongly recommended to use the `filelist` parameter to explicitly list the files inside the archive that should be signed. Signing is recursive; it will sign contents of any detectected `Zip`, `NuPkg` or `VSIX` files inside the uploaded one. After signing contents of the archive, the archive itself is signed if supported (currently `VSIX`).

### File list

Choose a reason for hiding this comment

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

This is argument, right? Could we create Argument section like https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-nuget-sign#arguments?

@dtivel
Copy link
Collaborator

dtivel commented Jan 7, 2023

@erdembayar, thanks for the feedback. It will not be overlooked.

I'm going to move this PR to draft state, as I think @clairernovotny mistakenly published it too early.

@dtivel dtivel marked this pull request as draft January 7, 2023 00:30
@clairernovotny clairernovotny marked this pull request as ready for review January 20, 2023 18:03
@clairernovotny
Copy link
Member Author

I'd like to get this merged asap so there's some docs on the cli usage. We can continue to update the formatting and contents of this doc after.

@erdembayar
Copy link

I'd like to get this merged asap so there's some docs on the cli usage. We can continue to update the formatting and contents of this doc after.

Could you please create tracking issue for improvement? Then I can approve it.

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