-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
chore(manager): move the manager and rename the repository to outline_apps
#1834
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1834 +/- ##
======================================
Coverage 32% 32%
======================================
Files 45 45
Lines 2609 2609
Branches 337 337
======================================
Hits 859 859
Misses 1750 1750
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
outline-apps
outline-apps
outline-frontend
outline-frontend
outline-apps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Manager refers to the install script in the outline-server repo. You must update that reference. We also need to update any docs we have referring to the install script location.
After the move, we should probably update the script in the original location with an error saying where the new script it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, where is it referenced in here? I can't seem to find it. Or are you saying this script should remain in the server repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/search?q=repo%3AJigsaw-Code%2Foutline-server%20install_script&type=code
Most critically, we have it on the UI: https://github.com/Jigsaw-Code/outline-server/blob/a62b25b54cf97770645d3fb601f601d2c92006be/src/server_manager/web_app/ui_components/outline-manual-server-entry.ts#L328
So we will have to keep it there for some time until everyone migrate. But also update all the docs and references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to double check if this contains all the latest changes to the scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of the files you are adding, which ones did you modify from the original? It's impossible to tell without a diff.
Also, please make sure you included all the recent changes:
https://github.com/Jigsaw-Code/outline-server/pulls?q=is%3Apr+is%3Aclosed
.gitignore
Outdated
@@ -26,3 +27,5 @@ tools/smartdnsblock/bin/* | |||
!tools/smartdnsblock/bin/*.exe | |||
universal.apk | |||
xcuserdata/ | |||
do_install_script.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this in the root?
I'd rather keep the .gitignore localized in the respective folder. Create the file there instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you omit the slash, it ignores anything called do_install_script.ts
- I'll make it more specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create a .gitignore in the directory where do_install_script.ts is created, so it's localized there.
I'd say same for /server_manager
to ignore the node_modules.
It's easier to manage these if they are in context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I prefer that, even
Sure, let me create a couple diffs. |
outline-apps
outline_apps
fair warning, this isn't too helpful either, as the format hook here is different than that of the manager - most of it is formatting changes |
btw, turns out outline-i18n isn't used in the manager: https://github.com/search?q=repo%3AJigsaw-Code%2Foutline-server%20outline-i18n&type=code also, do you mind if I catch up the changes in a follow up PR? otherwise I'll keep having to play catch up as we make changes to the move. we can do the catch up, rename this repo, and delete the manager from the other repo live as a team. |
Could we block further merges on the manager? |
Like, just not merge anything that touches the manager? Or write a check to enforce that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really need to know which, of the added files, you edited, so I know what to review.
.gitignore
Outdated
@@ -26,3 +27,5 @@ tools/smartdnsblock/bin/* | |||
!tools/smartdnsblock/bin/*.exe | |||
universal.apk | |||
xcuserdata/ | |||
do_install_script.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create a .gitignore in the directory where do_install_script.ts is created, so it's localized there.
I'd say same for /server_manager
to ignore the node_modules.
It's easier to manage these if they are in context.
By the way, were you able to successfully run the manager and create a server? |
I have an idea - I can run our formatter against the server repo and then do the diff after that. First I will fix the install script and confirm I can create a server |
I don't think we need to formalize it with a check, we're small enough to manage that informally? Unless it's easy to configure in GitHub somehow. |
@jyyi1 fyi |
@daniellacosse I just need to know the places where you changed code, ignore formatting. If you had branches after the copy, but before and after the changes, it would be ideal. Otherwise, knowing the files you changed would help. BTW, the PR title needs to be updated. |
I hear you, I just already forgot what I modified. The formatting occurs in the git hook, so there's no unformatted state. Let me crawl through the commits to try to jog my memory. |
Okay @fortuna, I think in the first commit I just copied everything over, and in the subsequent commits I got it to work. In those commits:
|
outline_apps
outline_apps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for the diff, I took a look at the most relevant files that changed and all looks good.
Please make sure the app still works with no runtime errors.
@@ -0,0 +1,24 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to match the original behavior. We should consider factoring out the common settings across projects at some point, like we have in outline-server.
This way we'll be able to share web code and build scripts, and tools between the projects (like the storybook!)
I also have a draft PR up that moves the client into its own folder! #1838