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

feat: github app token instead of pat for git sync #5279

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

alpetric
Copy link
Contributor

@alpetric alpetric commented Feb 13, 2025

Important

Add /workspaces/get_github_app_token endpoint to obtain GitHub app tokens, with implementation returning a not implemented error for OSS.

  • API Changes:
    • Added /workspaces/get_github_app_token endpoint in openapi.yaml for obtaining GitHub app tokens.
    • Updated global_service() in workspaces.rs to include the new route.
  • Functionality:
    • Implemented get_github_app_token() in workspaces_ee.rs, returning a "Not implemented on OSS" error.

This description was created by Ellipsis for 858d9eb. It will automatically update as commits are pushed.

Copy link

cloudflare-workers-and-pages bot commented Feb 13, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: cb4afb1
Status: ✅  Deploy successful!
Preview URL: https://40568d66.windmill.pages.dev
Branch Preview URL: https://alp-github-app.windmill.pages.dev

View logs

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to fe39183 in 2 minutes and 28 seconds

More details
  • Looked at 87 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. backend/windmill-api/src/openapi.yaml:700
  • Draft comment:
    Consider adding more descriptive documentation and security details for the new '/get_github_app_token' endpoint. E.g., what kind of authentication should be provided and more description on what the ‘job_token’ represents.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. backend/windmill-api/src/workspaces.rs:153
  • Draft comment:
    The route mapping for '/get_github_app_token' has been added. Verify that proper authorization logic is applied either in the endpoint handler or via middleware to ensure only allowed users can call it.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to verify the authorization logic, which is against the rules as it asks for confirmation or verification. It does not provide a specific suggestion or point out a specific issue with the code.
3. backend/windmill-api/src/workspaces_ee.rs:17
  • Draft comment:
    The 'get_github_app_token' function currently returns an internal error. If this is a placeholder for EE implementations, consider adding a TODO or comment to clarify that the functionality will be implemented in the enterprise edition.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The error message "Not implemented on OSS" is already self-documenting. The file name includes '_ee' indicating Enterprise Edition functionality. This appears to be a consistent pattern in the codebase for handling EE features in the OSS version. Adding a TODO would be redundant and doesn't suggest any actual code changes needed.
    Perhaps there's a company standard for TODO comments in EE placeholder functions that I'm not aware of. The suggestion might help new developers understand the codebase better.
    The existing error message and file naming convention already provide clear documentation. If there was a company standard for TODOs, it would likely already be present in the existing edit_auto_invite function above.
    Delete the comment as it suggests adding redundant documentation when the code is already clear and follows established patterns in the codebase.
4. backend/windmill-api/src/workspaces.rs:153
  • Draft comment:
    If GitHub App token functionality is meant only for Enterprise, consider gating the route with a feature flag rather than exposing it in the OSS build.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    The "_ee" suffix in the module path (workspaces_ee) strongly suggests this is already gated behind enterprise features. The comment is suggesting something that may already be implemented in the enterprise module. Without seeing the workspaces_ee module code, I can't be 100% certain, but the naming convention strongly indicates this is already properly gated.
    I don't have access to the workspaces_ee module code to verify if it's already feature-gated internally. The naming convention alone may not be sufficient evidence.
    While I don't have the enterprise module code, Rust naming conventions are very consistent and "_ee" is a clear indicator of enterprise edition code. The suggestion would add redundant gating.
    The comment should be deleted as it suggests adding enterprise feature gating that is likely already handled by the _ee module structure.
5. backend/windmill-api/src/workspaces_ee.rs:17
  • Draft comment:
    Instead of returning an internal error, consider using a more specific 'Not Implemented' (e.g. HTTP 501) error and clarify that GitHub App token retrieval is only available in Enterprise.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_LvdDXXcs3oIgsKHh


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 03fb830 in 40 seconds

More details
  • Looked at 19 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. backend/windmill-api/src/workspaces.rs:153
  • Draft comment:
    A new route for '/get_github_app_token' has been added. Ensure that the implementation in crate::workspaces_ee::get_github_app_token validates the token correctly and handles any security concerns. Also consider adding tests specific to the GitHub App token path.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. backend/windmill-api/src/workspaces.rs:857
  • Draft comment:
    In the enterprise branch's edit_git_sync_config function, verify that using a GitHub App token (instead of a PAT) is fully integrated in the serialization and update logic. Double-check that any token‐related fields are correctly handled and sensitive information is protected.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. backend/windmill-api/src/workspaces.rs:1378
  • Draft comment:
    The SQL query here appears unchanged except for formatting. Given the PR title, ensure the GitHub App token changes for git sync are implemented in the appropriate module.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_GcjW5HI22ED25SyR


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 858d9eb in 54 seconds

More details
  • Looked at 15 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. backend/windmill-api/src/workspaces.rs:1383
  • Draft comment:
    Only whitespace/indentation changes in the SQL query. No functional change observed.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. backend/windmill-api/src/workspaces.rs:1383
  • Draft comment:
    Minor whitespace reformatting in the SQL query. These changes are cosmetic—please confirm they align with the project's SQL formatting conventions.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None

Workflow ID: wflow_5YCHYvp4YFdc0Pt2


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@rubenfiszel rubenfiszel force-pushed the main branch 2 times, most recently from 5cdc1b8 to 5e22690 Compare February 17, 2025 16:56
@rubenfiszel rubenfiszel force-pushed the main branch 8 times, most recently from 0d0ada6 to 1ef482e Compare February 18, 2025 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant