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.

Sign up for GitHub

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

Jump to bottom

setup_secrets: Pass -clear as switch #194

Merged
merged 3 commits into from
Sep 14, 2023
Merged

setup_secrets: Pass -clear as switch #194

merged 3 commits into from
Sep 14, 2023

Conversation

amorask-bitwarden
Copy link
Contributor

@amorask-bitwarden amorask-bitwarden commented Sep 13, 2023 •
edited
Loading

Objective

While going through the onboarding, I had to reset my user secret several times. To do so, I was trying to use the -clear parameter as specified in the docs: pwsh setup_secrets.ps1 -clear:$True

However, I noticed that this part of the script was never being hit.

I changed $True to true and it started to work:

image

@amorask-bitwarden amorask-bitwarden requested a review from a team as a code owner September 13, 2023 19:55
@bitwarden-bot
Copy link

bitwarden-bot commented Sep 13, 2023 •
edited
Loading

Logo
Checkmarx One – Scan Summary & Details – 7700b904-8689-44f7-9b67-6445efe371d8

No New Or Fixed Issues Found

@withinfocus
Copy link
Contributor

Is it the casing? https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_booleans suggests lowercase T.

@amorask-bitwarden
Copy link
Contributor Author

Is it the casing? https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_booleans suggests lowercase T.

@withinfocus I thought that too, but I couldn't trigger the clear with $true either:
image

Hinton
Hinton previously approved these changes Sep 14, 2023
View reviewed changes
@Hinton Hinton dismissed their stale review September 14, 2023 09:31

Need more investigation

@Hinton
Copy link
Member

Hinton commented Sep 14, 2023

What happens if you try $true, $false in a powershell terminal?

image

@amorask-bitwarden
Copy link
Contributor Author

What happens if you try $true, $false in a powershell terminal?

image

@Hinton That works:
image

So my guess is that when passing the parameters in from a unix shell to the pwsh executable, you have to use true, but if you're already in a PowerShell context, it's $True. Do you think it's worth splitting the documentation at that point for Mac/Linux vs. Windows?

@Hinton
Copy link
Member

Hinton commented Sep 14, 2023 •
edited
Loading

I switched the bool to switch which removes the need to include :$True. bitwarden/server#3269 Can you update the docs to note that it's a switch and doesn't need any value?

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Sep 14, 2023 •
edited
Loading

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 739d98a
Status: ✅  Deploy successful!
Preview URL: https://242b8692.contributing-docs.pages.dev
Branch Preview URL: https://setup-secrets-clear-param.contributing-docs.pages.dev

View logs

@amorask-bitwarden amorask-bitwarden changed the title setup_secrets: Pass -clear boolean as true setup_secrets: Pass -clear as switch Sep 14, 2023
@amorask-bitwarden
Copy link
Contributor Author

I switched the bool to switch which removes the need to include :$True. bitwarden/server#3269 Can you update the docs to note that it's a switch and doesn't need any value?

@Hinton Updated in the rebased commit: 4e4a299

@amorask-bitwarden @Hinton
Change flag to switch
39c2dbe 
Co-authored-by: Oscar Hinton <[email protected]>
cturnbull-bitwarden
cturnbull-bitwarden approved these changes Sep 14, 2023
View reviewed changes
Copy link

@cturnbull-bitwarden cturnbull-bitwarden left a comment

Choose a reason for hiding this comment

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

👍 Looks good!

@amorask-bitwarden amorask-bitwarden merged commit 9387142 into master Sep 14, 2023
@amorask-bitwarden amorask-bitwarden deleted the setup-secrets-clear-param branch September 14, 2023 12:56
Hinton added a commit that referenced this pull request Oct 2, 2023
@Hinton
Merge branch 'master' of github.com:bitwarden/contributing-docs into …
8f1b733 
…ps/migrations

* 'master' of github.com:bitwarden/contributing-docs: (37 commits)
  chore(deps): update dependency cspell to v7.3.7 (#206)
  [PM-4161] Fix build command on README.md (#207)
  Fixed typo in csharp.md (#180)
  Update our EDD process documentation (#166)
  chore(deps): update actions/checkout action to v4.1.0 (#204)
  chore(deps): lock file maintenance (#205)
  fix(deps): update npm minor to v2.4.3 (#203)
  use dash when running self-hosted dotnet profile (#202)
  chore(deps): update actions/checkout action to v4 (#191)
  chore(deps): lock file maintenance (#201)
  chore(deps): update npm minor (#195)
  fix(deps): update dependency docusaurus-lunr-search to v3 (#200)
  setup_secrets: Pass `-clear` as switch (#194)
  Update KeyConnector docs for ARM Macs (#171)
  Lock file maintenance (#193)
  Update dependency cspell to v7.3.5 (#192)
  Update dependency ubuntu to v22 (#174)
  Update gh minor (#181)
  Feature flag documentation updates and mentions about new local override capabilities (#187)
  Update npm minor (#188)
  ...

# Conflicts:
#	custom-words.txt
#	docs/contributing/database-migrations/edd.mdx
#	docs/contributing/database-migrations/index.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hinton Hinton Hinton left review comments

@cturnbull-bitwarden cturnbull-bitwarden cturnbull-bitwarden approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@amorask-bitwarden @bitwarden-bot @withinfocus @Hinton @cturnbull-bitwarden