Skip to content
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

refactor internals, add initial gitlab support #121

Merged
merged 27 commits into from
May 8, 2024

Conversation

colemickens
Copy link
Member

  1. Refactored most of flakehub-push's internals:
    • There were a lot of places where we were bulk copying around values (such that we had to disable clippy lints), often times variables names differed on each side, leading to, for me, quite a bit of confusion
    • There's still at least two places where we branch off for GitLab/GitHub. This could potentially be improved in the future, but the way that we fill from CLI, backfill from env, fill from API, etc,... it wasn't as trivial as I'd hoped.
    • There's an attempt to prevent confusion between using the initial cli-provided values, and the new PushContext values that are the result of the various processing/backfilling we do.
  2. Added initial, albeit limited GitLab API support
    • Question: how to handle commit_count given that I don't think GitLab gives us a way to get it. I think there's a small additional logic change I can make to at least make commit_count optional for GitLab.

@colemickens colemickens force-pushed the colemickens/refactor-for-gitlab branch from f44e5c1 to 4056106 Compare April 17, 2024 18:23
src/error.rs Outdated Show resolved Hide resolved
src/flakehub_client.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
// - also checked the graphql api pretty extensively
// -- not finding anything obvious, not seeing the equivalent of the related github graphql query

// spdx_expression: can't find any evidence GitLab tries to surface this info
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not expecting this to be done now, but maybe we should leave a note about employing some library that allows us to detect the SPDX at least from a LICENSE{,.md} file.

src/push_context.rs Outdated Show resolved Hide resolved
src/push_context.rs Outdated Show resolved Hide resolved
src/push_context.rs Outdated Show resolved Hide resolved
src/push_context.rs Outdated Show resolved Hide resolved
src/flakehub_auth_fake.rs Outdated Show resolved Hide resolved
src/flakehub_auth_fake.rs Outdated Show resolved Hide resolved
src/push_context.rs Outdated Show resolved Hide resolved
@colemickens colemickens force-pushed the colemickens/refactor-for-gitlab branch from 21e5373 to 912e883 Compare May 8, 2024 17:25
@colemickens colemickens force-pushed the colemickens/refactor-for-gitlab branch from 912e883 to 2c53fac Compare May 8, 2024 21:36
src/git_context.rs Outdated Show resolved Hide resolved
src/push_context.rs Outdated Show resolved Hide resolved
src/push_context.rs Outdated Show resolved Hide resolved
src/push_context.rs Outdated Show resolved Hide resolved
src/push_context.rs Show resolved Hide resolved
src/push_context.rs Outdated Show resolved Hide resolved
@colemickens colemickens merged commit 925b381 into main May 8, 2024
6 checks passed
@colemickens colemickens deleted the colemickens/refactor-for-gitlab branch May 8, 2024 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants