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

update readme documentation #365

Merged
merged 18 commits into from
Jul 10, 2023
Merged

update readme documentation #365

merged 18 commits into from
Jul 10, 2023

Conversation

gord5500
Copy link
Contributor

@gord5500 gord5500 commented Jul 6, 2023

Why

Main branch has no documentation currently

@github-actions github-actions bot added the size/L A large PR. Could this be broken up? label Jul 6, 2023
@gord5500 gord5500 marked this pull request as ready for review July 6, 2023 13:56
README.md Outdated

## configure

The `bmx configure` command creates or updates the global BMX configuration file, located at `~/.bmx/config`. This file can store your Okta organization, username, and your AWS default session duration. Okta account sessions are saved when a configuration file is present. For security reasons, it's recommended to avoid running `bmx configure` or creating a global configuration file on a machine used by multiple users.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Felt like configure needed a bit more than 1 sentence just for the fact about multi user machine and the worries we had with that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could maybe go into a usage section above this actually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But do we trust someone to read it if they are only linked directly to the configure section

Copy link
Member

@boarnoah boarnoah Jul 6, 2023

Choose a reason for hiding this comment

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

Agreed we should clarify what the "security reason" is. Since its intended as a feature.

For example: #175 thinks of it as a bug when it seems like a happy coincidence / or possibly intended but not documented in the original BMX.

Copy link
Member

Choose a reason for hiding this comment

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

We have this now:

Console.Error.WriteLine( "No config file found. Your Okta session will not be cached. " +
"Consider running `bmx configure` if you own this machine." );

which I think adequately addresses possible user confusion.

README.md Outdated Show resolved Hide resolved
README.md Outdated

To setup AWS credentials in PowerShell, use:
```PowerShell
bmx print --account account-name --role role-name | iex
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The copy code button seems kind of useless when we're adding dummy flag values in

Copy link
Contributor Author

@gord5500 gord5500 Jul 6, 2023

Choose a reason for hiding this comment

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

Or it's just feels like it'll be more work to replace

@gord5500
Copy link
Contributor Author

gord5500 commented Jul 6, 2023

Ah github doesn't care about the language in code blocks. Just turns to black anyways

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
gord5500 and others added 2 commits July 6, 2023 14:00
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
gord5500 and others added 2 commits July 6, 2023 14:28
Co-authored-by: Chenfeng Bao <[email protected]>
Co-authored-by: Chenfeng Bao <[email protected]>
Co-authored-by: Chenfeng Bao <[email protected]>
@cfbao
Copy link
Member

cfbao commented Jul 6, 2023

Can we use underscores (instead of dashes) for placeholder values? e.g.

bmx print --account account_name --role role_name | iex

Looking at the rendered view:
image
Dashes are red, and creates more noise to my eyes.
It's also easier for people to replace these placeholder values if we use underscores, as it'd be treated as a single word by most editors.

gord5500 and others added 2 commits July 6, 2023 14:49
@gord5500 gord5500 merged commit a2c8c48 into main Jul 10, 2023
2 checks passed
@gord5500 gord5500 deleted the update_readme branch July 10, 2023 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L A large PR. Could this be broken up?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants