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

Updating README Usability #13

Merged
merged 19 commits into from
Aug 3, 2021
Merged

Updating README Usability #13

merged 19 commits into from
Aug 3, 2021

Conversation

willmarks
Copy link
Collaborator

@willmarks willmarks commented Jul 30, 2021

Closes FA-6

About

More information is needed for each field so that usage can be more self-sufficient. I also reworked where a couple fields were required and optional. Notably, commit sha is now only required for build and build_deployment as it was not being used for deployment.

@willmarks willmarks requested a review from tovbinm July 30, 2021 23:31
@willmarks willmarks marked this pull request as ready for review July 30, 2021 23:31
README.md Outdated Show resolved Hide resolved
README.md Outdated
__Allowed Values:__ Success, Failed, Canceled, Queued, Running, Unknown, Custom

1. `FAROS_VCS_SOURCE` / `--vcs_source "<vcs_source>"`
The version control source system that stores the code that is being built (i.e. GitHub, GitLab, Bitbucket) Please note that this field is case sensitive. If you have a feed that connects to one of these sources, this name must match exactly to be correctly associated.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created #14 to actually flesh out these source systems etc to mitigate issues from capitalization etc. The PR is already kinda big tho so I was just going to do it in a subsequent PR.

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
Copy link
Contributor

@tovbinm tovbinm left a comment

Choose a reason for hiding this comment

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

I do prefer table representation more, since it's more compact. I think you could have done it in a table as:
Argument | Description | Required | Default | Allowed Values

README.md Outdated
| Flag | Description |
| ------------- | -------------------------------------- |
| --dry_run | Print the event instead of sending. |
| -s / --silent | Unexceptional output will be silenced. |
Copy link
Contributor

Choose a reason for hiding this comment

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

just --silent is enough

Copy link
Contributor

@tovbinm tovbinm left a comment

Choose a reason for hiding this comment

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

lgtm!!!

@@ -22,13 +22,14 @@ FAROS_GRAPH_DEFAULT="default"
FAROS_URL_DEFAULT="https://prod.api.faros.ai"
FAROS_ORIGIN_DEFAULT="Faros_Script_Event"
FAROS_SOURCE_DEFAULT="Faros_Script"
FAROS_APP_PLATFORM_DEFAULT="NA"
FAROS_APP_PLATFORM_DEFAULT=""
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shubha requested that I change this default to "" so I just included it as it also required a readme change.

@willmarks willmarks merged commit b151963 into main Aug 3, 2021
@tovbinm tovbinm deleted the wm/readme branch August 3, 2021 18:50
@tovbinm
Copy link
Contributor

tovbinm commented Aug 3, 2021

@willmarks please create a new release

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