-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: new Zinnia API stationId
#520
Conversation
.gitignore
Outdated
@@ -1 +1,2 @@ | |||
/target | |||
*.DS_Store |
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.
*.DS_Store | |
*.DS_Store | |
@@ -178,6 +179,7 @@ mod tests { | |||
cache_root: temp.join("cache").to_string_lossy().into(), | |||
state_root: temp.join("state").to_string_lossy().into(), | |||
wallet_address: "f1test".to_string(), | |||
station_id: "zinnia-dev".to_string(), |
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 that only passing through the environment variable "station_id" isn't ideal, as Zinnia ideally doesn't know anything about Station. Instead there should be a way to pass any key value pair into the module context. I believe that implementing this is out of scope for this PR, so just noting here that I would like to improve this in the future.
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.
Nice! The changes look great. 👏🏻
I'd like to change one minor detail; the rest LGTM. 👍🏻
Also let's wait for the CI results to ensure all is green.
@PatrickNercessian also please fix the build failures:
|
stationId
https://github.com/filecoin-station/zinnia/actions/runs/8692944647/job/23914200368?pr=520
To speed up the process, I changed the PR from "draft" to a regular one, updated the PR title and will proceed to land & publish this change. |
DRAFT: DO NOT MERGE
Links:
space-meridian/roadmap#96