Enterprise hosting support #53

mattia · 2023-10-04T06:23:08Z

Add the ability to specify an GitHub Enterprise Server URL so that users can use custom instances of self-hosted GitHub. The runner type enterprise is added so enterprises hosted on GitHub Cloud can add runners to the whole enterprise

Fixes #27

mattia · 2023-10-04T06:24:26Z

This PR is very much a WIP, there are a bunch of things to do before it is ready (code cleanup, duplication, etc.) but we need to verify that it is working before all and at the moment I don't have any ability to do it. Let's hope the community can share some feedback

simonbs · 2023-10-06T08:05:16Z

@mattia Thanks a lot for opening this PR! I think it would be great to have this functionality in Tartelet. I'll make sure to review it once I have the time 😊

mattia · 2023-10-06T10:17:44Z

...ources/VirtualMachineResourcesServiceEphemeral/VirtualMachineResourcesServiceEphemeral.swift

+                throw VirtualMachineResourcesServiceEphemeralError.invalidRunnerURL
+            }
+//            SEE https://docs.github.com/en/enterprise-server@3.10/rest/actions/self-hosted-runners?apiVersion=2022-11-28#create-a-registration-token-for-an-enterprise
+            return baseURL.appending(path: enterpriseName, directoryHint: .notDirectory)


This should be checked, from my understating of the API the base URL should be set on your instance URL (and that's what I have done in code), but in the example link they hardcode github.com. If anyone can confirm/update me on this, please let me know

hoon-prestolabs · 2023-12-21T10:51:37Z

Any update?

mattia · 2023-12-21T20:27:04Z

Any update?

Hi @hoon-prestolabs! Unfortunately I don't have any news regarding this. The basic functionality is implemented and should work fine but I'm leaving the PR in draft because I don't want to propose this change without a feedback from users with this configuration.
If you are using GitHub Enterprise Server your feedback on this change would be welcome!

mattia · 2023-12-21T20:31:22Z

(rebased the branch on main just to keep up with recent updates)

simonbs

Once again, thank you for opening this PR and thank you for your patience. I apologize for the delay in responding.

I have added a few comments to the code changes. Some of the comments include required changes, such as ensuring texts are localized, and others include suggestions for improvements.

My main concern with merging these changes right now is that I am unable to test them myself, as I do not have access to an enterprise GitHub setup. I am also concerned about whether the new Version setting allows users to select an invalid combination of settings.

For example, is it valid to select the "github self hosted" and "organization" settings as shown in the screenshot below?

And is it valid to select the "github.com" and "enterprise" settings as shown below?

It is crucial that users cannot select an invalid combination of settings. However, since I have limited knowledge of GitHub's enterprise offerings, I am uncertain if this is achievable.

I am happy to continue with this PR if someone can verify that all combinations of settings are valid and that Tartelet exhibits the expected behavior.

simonbs · 2024-01-03T21:12:04Z

Packages/Settings/Sources/SettingsUI/Internal/GitHubSettings/GitHubPrivateKeyPicker.swift

@@ -12,6 +12,8 @@ struct GitHubPrivateKeyPicker: View {
            L10n.Settings.Github.PrivateKey.Scopes.organization
        case .repo:
            L10n.Settings.Github.PrivateKey.Scopes.repository
+        case .enterpriseServer:
+            "Check permissions: `manage_runners:enterprise`"


This text needs to be localized and formatted similar to the other texts listing required permissions.

simonbs · 2024-01-03T21:12:26Z

Packages/Settings/Sources/SettingsUI/Internal/GitHubSettings/GitHubSettingsView.swift

+
+                if viewModel.version == .enterprise {
+                    TextField(
+                        "Self hosted URL",


This text needs to be localized and accessed through the L10n constant.

simonbs · 2024-01-03T21:12:32Z

Packages/Settings/Sources/SettingsUI/Internal/GitHubSettings/GitHubSettingsView.swift

+                    TextField(
+                        "Self hosted URL",
+                        text: $viewModel.selfHostedRaw,
+                        prompt: Text("Github Service raw value")


This text needs to be localized and accessed through the L10n constant.

simonbs · 2024-01-03T21:12:37Z

Packages/Settings/Sources/SettingsUI/Internal/GitHubSettings/GitHubSettingsView.swift

@@ -41,6 +56,13 @@ struct GitHubSettingsView: View {
                        prompt: Text(L10n.Settings.Github.RepositoryName.prompt)
                    )
                    .disabled(!viewModel.isSettingsEnabled)
+                case .enterpriseServer:
+                    TextField(
+                        "Enterprise name",


This text needs to be localized and accessed through the L10n constant.

simonbs · 2024-01-03T21:12:50Z

Packages/Settings/Sources/SettingsUI/Internal/GitHubSettings/GitHubSettingsView.swift

@@ -77,6 +99,19 @@ private extension GitHubRunnerScope {
            L10n.Settings.RunnerScope.organization
        case .repo:
            L10n.Settings.RunnerScope.repository
+        case .enterpriseServer:
+            "Enterprise"


This text needs to be localized and accessed through the L10n constant.

simonbs · 2024-01-03T21:18:57Z

Packages/GitHub/Sources/GitHubService/GitHubServiceVersion.swift

+  case dotCom
+  case enterprise(URL)


These enum cases don't seem to be used. Can we remove them and simplify this enum?

simonbs · 2024-01-03T21:21:49Z

Packages/Settings/Sources/SettingsUI/Internal/GitHubSettings/GitHubSettingsView.swift

+        case .dotCom:
+            return "github.com"
+        case .enterprise:
+            return "github self hosted"


I'm unfamiliar with enterprise GitHub setups and self-hosting so please correct me if I'm wrong but the enterprise enum case seems to refer to GitHub Enterprise Server. Would it make sense to rename the case and the title to be more explicit about this and avoid confusion with the enterprise runner scope?

simonbs · 2024-01-03T21:23:45Z

Packages/Settings/Sources/SettingsUI/Internal/GitHubSettings/GitHubSettingsViewModel.swift

+                let runnerScope = tuple.0
+                let organizationName = tuple.1
+                let ownerName = tuple.2
+                let repositoryName = tuple.3


This seems like a good opportunity to use tuple destructuring.

Suggested change

let runnerScope = tuple.0

let organizationName = tuple.1

let ownerName = tuple.2

let repositoryName = tuple.3

let (runnerScope, organizationName, ownerName, repositoryName) = tuple

simonbs · 2024-01-03T21:26:28Z

Packages/Settings/Sources/SettingsUI/Internal/GitHubSettings/GitHubSettingsView.swift

@@ -14,6 +14,21 @@ struct GitHubSettingsView: View {
    var body: some View {
        Form {
            Section {
+                Picker("Version", selection: $viewModel.version) {


This text needs to be localized and accessed through the L10n constant.

simonbs · 2024-01-03T21:27:28Z

Packages/Settings/Sources/SettingsUI/Internal/GitHubSettings/GitHubSettingsView.swift

+                    ForEach(GitHubServiceVersion.Kind.allCases, id: \.self) { scope in
+                        Text(scope.title)
+                    }
+                }


Would it make sense to turn this picker into a segmented control to align with the Runner Scope setting?

Suggested change

}

}

.pickerStyle(.segmented)

mattia mentioned this pull request Oct 4, 2023

Allow GitHub URLs to be set in settings to support GitHub Enterprise and Enterprise Cloud #27

Open

mattia commented Oct 6, 2023

View reviewed changes

mattia added 4 commits December 21, 2023 21:29

Add ability to store custom GitHub URL

345e831

Update baseURL used for GitHub service

5f0c908

Add ability to use custom enterprise server

48578a9

Add handling of enterprise self hosted runner

567b8a6

mattia force-pushed the feature/enterprise-hosting-support branch from 3fd34eb to 567b8a6 Compare December 21, 2023 20:30

simonbs requested changes Jan 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enterprise hosting support #53

Enterprise hosting support #53

mattia commented Oct 4, 2023 •

edited

mattia commented Oct 4, 2023 •

edited

simonbs commented Oct 6, 2023

mattia Oct 6, 2023 •

edited

hoon-prestolabs commented Dec 21, 2023

mattia commented Dec 21, 2023

mattia commented Dec 21, 2023

simonbs left a comment

simonbs Jan 3, 2024

simonbs Jan 3, 2024

simonbs Jan 3, 2024

simonbs Jan 3, 2024

simonbs Jan 3, 2024

simonbs Jan 3, 2024

simonbs Jan 3, 2024

simonbs Jan 3, 2024

simonbs Jan 3, 2024

simonbs Jan 3, 2024

Enterprise hosting support #53

Are you sure you want to change the base?

Enterprise hosting support #53

Conversation

mattia commented Oct 4, 2023 • edited

mattia commented Oct 4, 2023 • edited

simonbs commented Oct 6, 2023

mattia Oct 6, 2023 • edited

Choose a reason for hiding this comment

hoon-prestolabs commented Dec 21, 2023

mattia commented Dec 21, 2023

mattia commented Dec 21, 2023

simonbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattia commented Oct 4, 2023 •

edited

mattia commented Oct 4, 2023 •

edited

mattia Oct 6, 2023 •

edited