Skip to content

Enable GitHub Enterprise Server#183

Merged
felipesu19 merged 5 commits intogithub:mainfrom
Olfi01:main
Jun 14, 2023
Merged

Enable GitHub Enterprise Server#183
felipesu19 merged 5 commits intogithub:mainfrom
Olfi01:main

Conversation

@Olfi01
Copy link
Copy Markdown

@Olfi01 Olfi01 commented May 8, 2023

This pull request implements #12
Depends on actions/languageservices#43

For GitHub Enterprise Server users, the plugin has the issue of not recognizing actions and shared workflows hosted only there. This pull request adds an option to use the already available authentication provider for GitHub Enterprise Server (official VSCode GitHub plugin).

@Olfi01 Olfi01 marked this pull request as ready for review May 12, 2023 13:06
@Olfi01 Olfi01 requested a review from a team as a code owner May 12, 2023 13:06
@Olfi01 Olfi01 marked this pull request as draft May 12, 2023 17:09
@Olfi01 Olfi01 marked this pull request as ready for review May 12, 2023 17:40
@Olfi01 Olfi01 mentioned this pull request May 12, 2023
@cschleiden
Copy link
Copy Markdown
Member

cschleiden commented May 15, 2023

Thank you for this. Unfortunately for full GHES support it's not just about authenticating and making calls to the right API base url. The biggest issue to solve is versioning.

Not every customer is running the latest GHES version. And the latest released GHES version does not always support the same features as github.com. So some APIs that the extension relies on might not be available on the connected GHES version. Or the GitHub Actions workflow schema that ships with the extension does not match the features supported by the connected GHES version.

Ideally, we detect the GHES version and select the correct feature set, but this requires more work across the language services and the extension itself.

We might be able to add what you're proposing here as something that's unsupported (but still available) but that requires some more discussion.

@parkl-ee
Copy link
Copy Markdown

parkl-ee commented May 16, 2023

I'd love to see this added as well, even as an "experimental/unsupported" feature.

@Olfi01
Copy link
Copy Markdown
Author

Olfi01 commented May 17, 2023

@cschleiden I understand, I've ran into one of those limitations myself. Using a GHES version 3.5, my particular setup doesn't yet support variables. While right now this is the only meaningful difference I could identify, you are right that for a supported feature my contribution isn't covering nearly enough ground.

About the unsupported feature question, I was considering to publish a fork of the plugin, with a notice that it's very experimental and a recommendation to use the official plugin if you don't need GHES support, for people like @ParksBra. What do you think about that? Would you prefer to publish such a fork yourself?

@felipesu19
Copy link
Copy Markdown
Contributor

felipesu19 commented May 23, 2023

After internal discussion, we're going to allow this as an experimental/unsupported (for now) feature!. I'll approve, if CI passes, we'll merge, if not, feel free to fix ci issues and then we'll re-approve and merge as needed.

Also adding a note to the Readme in this pr

felipesu19
felipesu19 previously approved these changes May 23, 2023
Copy link
Copy Markdown
Member

@cschleiden cschleiden left a comment

Choose a reason for hiding this comment

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

Thanks for this change, couple naming comments and CI/Linting complains. Please take a look, happy to approve then.

@Olfi01
Copy link
Copy Markdown
Author

Olfi01 commented May 24, 2023

@cschleiden @felipesu19 CI will still fail because the PR depends on a newer, not yet released version of lanugageserver (actions/languageservices#43)

@Olfi01 Olfi01 requested a review from cschleiden May 26, 2023 07:25
@Olfi01
Copy link
Copy Markdown
Author

Olfi01 commented Jun 12, 2023

@cschleiden Is there still something I need to do before this can be merged? I can't release the changes to the languageserver myself

felipesu19
felipesu19 previously approved these changes Jun 13, 2023
Copy link
Copy Markdown
Contributor

@felipesu19 felipesu19 left a comment

Choose a reason for hiding this comment

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

Approving, will wait to run ci until language server changes are merged.

@felipesu19
Copy link
Copy Markdown
Contributor

Gonna trust that with the language server changes in place this will work.

@felipesu19 felipesu19 merged commit f3b069f into github:main Jun 14, 2023
@elbrenn
Copy link
Copy Markdown
Collaborator

elbrenn commented Jun 15, 2023

@Olfi01 thank you for your contributions here ✨ I've tried out your changes locally and am excited to deploy them! We are planning a release early next week.

Like you mentioned, there are undesirable behaviors related to features not present in whatever version of GHES a user is running. We'll keep #12 open to track GHES support with versioning.

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.

6 participants