-
Notifications
You must be signed in to change notification settings - Fork 4
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
api: Recognize sapphire, cipher as valid Layer values #427
Conversation
Makefile
Outdated
test-e2e-regression: export TZ=UTC | ||
test-e2e-regression: oasis-indexer | ||
./oasis-indexer --config tests/e2e_regression/e2e_config.yml analyze | ||
TZ=UTC ./oasis-indexer --config tests/e2e_regression/e2e_config.yml analyze |
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.
Unrelated. No-op change, just makes the command easier to copy-paste out of the Makefile
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 think we actually need to move this to the line that instantiates the api server in run.sh
, since the timestamp is formatted by the api server.
@# The result of the "spec" test is a special case. It should always match the | ||
@# current openAPI spec file, so we symlink it to avoid having to update the expected | ||
@# output every time the spec changes. |
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 reason I abstracted the inline command form run.sh
into this accept-e2e-regressions
rule in the Makefile. The PR caused e2e regressions to fail just because it changed the openapi JSON (but not the e2e expected file). It would be awkward if every change to the spec produced two equivalent diffs: one in the actual spec, and one in the e2e expected file.
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.
as described
@$(ECHO) "$(CYAN)*** Indexer finished; starting api tests...$(OFF)" | ||
./tests/e2e_regression/run.sh | ||
|
||
# Accept the outputs of the e2e tests as the new expected outputs. | ||
accept-e2e-regression: |
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.
add to .PHONY?
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.
🤷 We don't really use/need it. Added, and half-heartedly added several other e2e-related targets that were missing, but didn't make a careful pass to ensure every relevant target is listed under .PHONY.
763fdec
to
28f3ba7
Compare
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.
Thanks! One comment but lgtm otherwise!
Makefile
Outdated
test-e2e-regression: export TZ=UTC | ||
test-e2e-regression: oasis-indexer | ||
./oasis-indexer --config tests/e2e_regression/e2e_config.yml analyze | ||
TZ=UTC ./oasis-indexer --config tests/e2e_regression/e2e_config.yml analyze |
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 think we actually need to move this to the line that instantiates the api server in run.sh
, since the timestamp is formatted by the api server.
28f3ba7
to
b779852
Compare
Good catch @Andrew7234 (I can't reply to the thread)! Moved |
The
IsValid()
function is used to validate APIs; before this bugfix PR, https://index-staging.oasislabs.com/v1/sapphire/stats/active_accounts returned"Invalid format for parameter layer: not a valid enum value: sapphire"
Reported by @csillag -- thanks!