-
Notifications
You must be signed in to change notification settings - Fork 10
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
Shell script friendly API and OpenAPI doc #46
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: AbstractionFactory <179820029+abstractionfactory@users.noreply.github.com>
Deploying opentofu-get with
|
Latest commit: |
f851020
|
Status: | ✅ Deploy successful! |
Preview URL: | https://7030119a.opentofu-get.pages.dev |
Branch Preview URL: | https://api.opentofu-get.pages.dev |
Signed-off-by: AbstractionFactory <179820029+abstractionfactory@users.noreply.github.com>
Signed-off-by: AbstractionFactory <179820029+abstractionfactory@users.noreply.github.com>
Signed-off-by: AbstractionFactory <179820029+abstractionfactory@users.noreply.github.com>
Signed-off-by: AbstractionFactory <179820029+abstractionfactory@users.noreply.github.com>
Signed-off-by: AbstractionFactory <179820029+abstractionfactory@users.noreply.github.com>
This looks great. Only thing to add is, since it is now an interactive web server I would suggest always returning a JSON response (unless otherwise state like with the shell script). This would make it easer to handle error cases. For example when trying to fetch version |
Hi @ProbstDJakob the files are still just statically generated, there is no interactivity here, otherwise I would have added an error response. Referencing a non-existent version would return with a 404 status code. We could add a web worker for the purposes of only the error responses maybe? @Yantrio you know more about web workers, what do you think? |
Oh ok, then my assumption was wrong, but in most static web servers you can specify a default response (file). This could then contain For example for nginx it is documented here: https://nginx.org/en/docs/http/ngx_http_core_module.html#error_page But in either cases I have not tested how it handles non-HTML files, i.e. if it correctly sets the |
@ProbstDJakob we are using Cloudflare, so I believe that would require a worker. I'll discuss with @Yantrio what's possible here, he knows the Cloudflare ecosystem better than I do. |
@abstractionfactory this is now unblocked |
@@ -35,6 +38,26 @@ func githubResponseToIndex(response github.ReleasesResponse) *Index { | |||
return files | |||
}(), | |||
} | |||
result.Versions[i] = verStruct | |||
parts := strings.Split(ver, "-") |
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.
Would it make sense to use something like mod/semver for this parsing?
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 intentionally didn't because a lot more tools depend on how we name our versions, not everything goes that semver allows.
result[version.ID+"/index.html"], err = renderTemplate(releaseTemplate, version) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to render release template for version %s: %w", version.ID, err) | ||
} | ||
} | ||
result["api.txt"] = []byte(strings.Join(versionIDList, "\n")) |
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.
Sug: versions.txt as it's not structured or similar to api.json
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 can do that, although in retrospect I'm not particularly happy with using the /api.json
either, it should have rather been /tofu/api/versions.json
or similar, but there's no fixing that now. Alternatively, we could also transition to the /api
endpoint and deprecate /tofu/api.json
, which would be much cleaner.
return nil, fmt.Errorf("failed to marshal version files API file for %s: %w", version.ID, err) | ||
} | ||
|
||
result["api/"+ver+".version.txt"] = []byte(version.ID) |
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.
What does the txt version of these actually give us / has it been requested?
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.
It lets shell scripts easily figure out what versions are available. JSON parsing usually depends on jq
being present, which is not a given, especially not in a CI environment. (Currently the install-opentofu.sh
make very few assumptions about the tools being present.) So, the user can specify install-opentofu.sh --version 1.7
and it will fetch the latest 1.7 version once using this API is implemented. This has been requested in #30.
Blocked by: #44This PR adds new API endpoints that are more convenient to use from shell scripts. This is a precursor to fixing #30.
API doc preview: https://api.opentofu-get.pages.dev/tofu/api/
Specifically, this exposes the following endpoints:
/tofu/api/{versionFragment}.version.txt
/tofu/api/{versionFragment}.files.txt
/tofu/api/{versionFragment}.version.json
/tofu/api/{versionFragment}.files.json
Where
versionFragment
can belatest
,1
,1.9
, or a full version. Furthermore, this updates the/tofu/api.json
to include a newlatest_versions
section for easier programmatic access to the latest versions.It also makes a Redoc page available at
/tofu/api/
for easier implementation.Note
This code still depends on the GitHub release order and patch releases should always be done in order.
Open questions
/tofu/api...
or should we move to/api
and deprecate/tofu/api.json
? Admittedly, the whole thing is kind of an API as shown in the API docs above.