-
Notifications
You must be signed in to change notification settings - Fork 7
improve STAC OpenAPI and service metadata #621
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
Open
fmigneault
wants to merge
1
commit into
master
Choose a base branch
from
stac-service-metadata
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should go in "VARS" since they'll break things if they're not set
(service-config.json will contain invalid URLs and the OPENAPI_URL and DOCS_URL paths will just be
/stacwhich may confuse the stac-app)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I though
VARSis for "you must provide them" andOPTIONAL_VARSfor "some default exists", and not overriding them will use the defaults. If overridden with inadequate values, it can break, and it is up to the dev not to provide garbage in.If that's not the case, then there are many actually breaking
OPTIONAL_VARSall over the repository, such as all_DOCKER,_VERSIONand_IMAGE(and many more) that create invalid docker compose if explicitly set empty.I think in this specific case, setting them to empty explicitly (for whatever reason) would still startup correctly, though indeed would lead to weird URLs.
Should the
VARS/OPTIONAL_VARSbe better defined, with review of existing definitions? It is not the first time this confusion occurred. The current descriptions leave them up to personal interpretation, notably because both can use defaults, "they do not have to be defined in 'env.local' if values are adequate", and empty is technically a defined variable, just not "set".birdhouse-deploy/birdhouse/default.env
Lines 48 to 51 in 2ef86b1
birdhouse-deploy/birdhouse/default.env
Lines 70 to 74 in 2ef86b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message when a VARS variable is empty is:
"Required variable ${v#$} is not set"and then the process exits. This is checked with the-ztest which checks for empty or unset strings.To me this has always implied that these variables must be set to a non-empty value in order for the stack to work properly.
I think that this is almost right but also it means that you cannot override the default VARS variables and make the value empty.
This is true as well
Yes please