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: Self hosted GitLab instances #957

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

munen
Copy link
Collaborator

@munen munen commented Jun 8, 2023

No description provided.

@munen
Copy link
Collaborator Author

munen commented Jun 8, 2023

@zaz has implemented using self hosted Gitlab instances as OAuth providers and sync-backends for organice 🙏

I have made a regression test and could still login to gitlab.com and make changes to a repo. I have also tested logging in to a self hosted instance and making changes to a repo.

@chasecaleb made the initial Gitlab integration. Would you like to give this PR a glance before we pull it in?

@chasecaleb also made a good point back in his original PR stating that if organice were to support self hosted instances of Gitlab, it would make sense to:

  • Add inputs at sign-in to allow the user to provide domain and OAuth client ID + secrets.
  • Provide/link to instructions for registering an OAuth application on their instance.

Otherwise people will be required to self host organice, as well, if they want to use a self hosted Gitlab. For "regular users" of Gitlab.com, these additional fields should not show up.

@zaz Would you be interested in doing that?

Copy link
Collaborator Author

@munen munen left a comment

Choose a reason for hiding this comment

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

It looks good to me apart from the one minor issue that I've addressed.

createGitlabOAuth().fetchAuthorizationCode();
} else {
evt.preventDefault();
alert('Project does not appear to be a valid gitlab.com URL');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@zaz I see that you've refactored the extraction of the projectId into the sync backend itself (function getProject). Is it still possible to have a validation if no projectId can be extracted as here? You've left a TODO here yourself^^

Copy link

Choose a reason for hiding this comment

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

Sure. Do you want the alert in gitLabProjectIdFromURL? Or would it be possible to move the alert into createGitlabOAuth? It might be nice to remove most of the checks in gitLabProjectIdFromURL and instead just attempt the OAuth and gracefully handle errors there, unless there would be a downside to that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can move it to createGitlabOAuth, I guess. It's a Redux action and those can have side effects.

@@ -113,15 +112,10 @@ function GitLab() {
const defaultProject = 'https://gitlab.com/your/project';
const [project, setProject] = useState(defaultProject);
const handleSubmit = (evt) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Minor issue here: yarn eslint will complain that evt is never used. I'm not removing it as long as the validation of the projectId is still in question.

@zaz
Copy link

zaz commented Jun 8, 2023

  • Add inputs at sign-in to allow the user to provide domain and OAuth client ID + secrets.

  • Provide/link to instructions for registering an OAuth application on their instance.

Otherwise people will be required to self host organice, as well, if they want to use a self hosted Gitlab. For "regular users" of Gitlab.com, these additional fields should not show up.

@zaz Would you be interested in doing that?

I probably can. I'll have to read the documentation on OAuth you emailed me because I'm still confused why you need to authenticate twice (first when you create the OAuth application and give the secret to your app, then again when you use the app for the first time).

This fork definitely needs some changes and a rebase before merging.

@zaz
Copy link

zaz commented Jun 14, 2023

I'm going to be very busy for the next few weeks. I do want to read the source code and read about OAuth and alternatives when I get time.

I don't like the idea of adding more clutter and complexity just to support such a specific thing. It's already odd that the options are Dropbox, Gitlab, and WebDAV: Two specific websites and one general standard.

Is it possible to support a wide swathe of the systems out there? E.g. Add git support with isomorphic-git? Or any other standardized way of syncing?

I will still work on this when I get chance (unless you've done it before then), but afterwards I would want to generalize it.

@zaz
Copy link

zaz commented Jun 14, 2023

Also, from what I understand, the current solution relies on environment variables, so on some static site servers it will not work, or will require manual configuration. For example, it seems that deploying to GitHub pages would not be trivial right now.

In general, it just seems simpler to hard-code the authentication codes as then deploying is as simple as uploading files to a web server.

Or am I missing something?

@munen
Copy link
Collaborator Author

munen commented Jun 20, 2023

Is it possible to support a wide swathe of the systems out there? E.g. Add git support with isomorphic-git? Or any other standardized way of syncing?

I have added an ADR on how to work with different synchronization back-ends: https://organice.200ok.ch/documentation.html#adr-003

@munen
Copy link
Collaborator Author

munen commented Jun 20, 2023

Also, from what I understand, the current solution relies on environment variables, so on some static site servers it will not work, or will require manual configuration. For example, it seems that deploying to GitHub pages would not be trivial right now.

You misunderstand how environment variables work in a Create React App application. Here are the upstream docs: https://create-react-app.dev/docs/adding-custom-environment-variables/

To make it short: The environment variables as implemented will work well when deploying to any static site host such as GitHub pages.

In general, it just seems simpler to hard-code the authentication codes as then deploying is as simple as uploading files to a web server.

Hard coding will by definition not make it simple, because you will complect code and configuration - resulting in complexity, not simplicity.

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.

2 participants