Remove non-token dependencies on GitHub Actions #109
Conversation
Hoverbear
force-pushed
the
even-less-github-dependencies
branch
from
1e0a334 to
22b8c0e
Compare
Hoverbear
force-pushed
the
even-less-github-dependencies
branch
from
22b8c0e to
a05a1bc
Compare
| @@ -338,16 +384,7 @@ impl FlakeHubPushCli { | |||
| PathBuf::new() | |||
| }; | |||
|
|
|||
| let repository = if let Some(repository) = &repository.0 { | |||
There was a problem hiding this comment.
Moved to populate_missing_from_github
| @@ -369,12 +406,6 @@ impl FlakeHubPushCli { | |||
| repository.clone() | |||
| }; | |||
|
|
|||
| let tag = if let Some(tag) = &tag.0 { | |||
There was a problem hiding this comment.
Moved to populate_missing_from_github
| git_root.clone() | ||
| } else if let Ok(github_workspace) = std::env::var("GITHUB_WORKSPACE") { |
There was a problem hiding this comment.
Moved to populate_missing_from_github
| .unwrap_or_else(|| "<none>".into()), | ||
| github_graphql_data_result.rev_count as usize | ||
| ); | ||
| commit_count = Some(github_graphql_data_result.rev_count as usize); |
There was a problem hiding this comment.
I could see us checking if it's len 1 or something but this feels safe.
| rolling, | ||
| rolling_minor.0, | ||
| github_graphql_data_result, |
There was a problem hiding this comment.
Getting this arg out of push_new_release was part of the main goal here.
| flake_metadata: serde_json::Value, | ||
| flake_outputs: serde_json::Value, | ||
| upload_name: String, | ||
| mirror: bool, | ||
| visibility: Visibility, | ||
| github_graphql_data_result: GithubGraphqlDataResult, |
There was a problem hiding this comment.
Getting this arg out of build was part of the main goal here.
| #[tracing::instrument(skip_all)] | ||
| pub(crate) async fn get_actions_id_bearer_token() -> color_eyre::Result<String> { | ||
| #[tracing::instrument(skip_all, fields(audience = tracing::field::Empty))] | ||
| pub(crate) async fn get_actions_id_bearer_token(host: &url::Url) -> color_eyre::Result<String> { |
There was a problem hiding this comment.
Change mostly included for help with testing.
| revision_info, | ||
| &repository, | ||
| revision, | ||
| commit_count, |
There was a problem hiding this comment.
I'm going to keep looking to see if I can answer this, but it seems like maybe we use local commit count, but most times that's over-ridden by getting it from github?
I'm just a bit nervous this exposes us to (running outside github) and then having rolling releases be broken there.
(I'm looking to see if I've missed/forgotten a more fundamental fix to the local commit count)
There was a problem hiding this comment.
Any time a github token is present we rely on Github (since it is likely more correct -- and if we're in GHA then it's always 1).
If the user is running the tool outside GHA then we can't necessarily rely on being able to ask Github -- so we need to rely on the locally discovered commit count (which may be wrong).
There was a problem hiding this comment.
(which may be wrong).
totally understood, I just feel like we sorta still have an underlying bug that is mostly masked because we only expect to work in github. Yet we have an explicit code path that we know might return bad data.
It almost feels like we should be erroring out without a commit count we return. It feels like it's a Maybe<T> almost, unless we're in GitHub.
again, not an issue with this PR but I don't want us to merge this, forget, and then re-expose ourselves to this bug in a new codepath.
This does quite a bit of logic changing to make us not require GitHub Actions for anything but the bearer token.
It is important to note that this (for now) refuses to run if
GITHUB_ACTIONis not set or--github-tokenis not passed. So this doesn't actually remove the need for Github Actions, it just sets the stage for us to add support (similar to #107 and #108).I believe some of the error messages are still a bit rough... Should give them a better pass before merge.