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

chore(manager): move the manager and rename the repository to outline_apps #1834

Merged
merged 15 commits into from
Feb 21, 2024

Conversation

daniellacosse
Copy link
Contributor

@daniellacosse daniellacosse commented Feb 6, 2024

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

server_manager/electron_app/fetch.ts Dismissed Show resolved Hide resolved
server_manager/electron_app/gcp_oauth.ts Dismissed Show dismissed Hide dismissed
Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6356371) 32% compared to head (b7fb05a) 32%.

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           
Flag Coverage Δ
apple 15% <ø> (ø)
maccatalyst 15% <ø> (ø)
macos 15% <ø> (ø)
unittests 32% <ø> (ø)
www 40% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@daniellacosse daniellacosse changed the title proposal(manager): move the manager proposal(manager): move the manager and rename the repository to outline-apps Feb 6, 2024
@daniellacosse daniellacosse changed the title proposal(manager): move the manager and rename the repository to outline-apps proposal(manager): move the manager and rename the repository to outline-frontend Feb 6, 2024
@daniellacosse daniellacosse requested a review from fortuna February 6, 2024 04:58
@daniellacosse daniellacosse changed the title proposal(manager): move the manager and rename the repository to outline-frontend proposal(manager): move the manager and rename the repository to outline-apps Feb 7, 2024
@daniellacosse daniellacosse marked this pull request as ready for review February 7, 2024 21:01
@daniellacosse daniellacosse requested a review from a team as a code owner February 7, 2024 21:01
Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

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.

@fortuna fortuna self-requested a review February 15, 2024 16:39
Copy link
Collaborator

@fortuna fortuna left a 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
Copy link
Collaborator

@fortuna fortuna Feb 15, 2024

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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.

Copy link
Contributor Author

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

package.json Show resolved Hide resolved
@daniellacosse
Copy link
Contributor Author

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

Sure, let me create a couple diffs.

@daniellacosse daniellacosse changed the title proposal(manager): move the manager and rename the repository to outline-apps proposal(manager): move the manager and rename the repository to outline_apps Feb 15, 2024
@daniellacosse
Copy link
Contributor Author

daniellacosse commented Feb 15, 2024

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

Sure, let me create a couple diffs.

migration.zip

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

@daniellacosse
Copy link
Contributor Author

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

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.

@sbruens
Copy link
Contributor

sbruens commented Feb 15, 2024

Could we block further merges on the manager?

@daniellacosse
Copy link
Contributor Author

Could we block further merges on the manager?

Like, just not merge anything that touches the manager? Or write a check to enforce that?

Copy link
Collaborator

@fortuna fortuna left a 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
Copy link
Collaborator

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.

@fortuna
Copy link
Collaborator

fortuna commented Feb 16, 2024

By the way, were you able to successfully run the manager and create a server?

@daniellacosse
Copy link
Contributor Author

I really need to know which, of the added files, you edited, so I know what to review.

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

@sbruens
Copy link
Contributor

sbruens commented Feb 16, 2024

Could we block further merges on the manager?

Like, just not merge anything that touches the manager? Or write a check to enforce that?

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.

@daniellacosse
Copy link
Contributor Author

Could we block further merges on the manager?

Like, just not merge anything that touches the manager? Or write a check to enforce that?

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

@fortuna
Copy link
Collaborator

fortuna commented Feb 16, 2024

@daniellacosse I just need to know the places where you changed code, ignore formatting.
For sure you had to edit some files to make the code work. We need to know what you had to modify.

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.

@daniellacosse
Copy link
Contributor Author

@daniellacosse I just need to know the places where you changed code, ignore formatting. For sure you had to edit some files to make the code work. We need to know what you had to modify.

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.

@daniellacosse
Copy link
Contributor Author

@daniellacosse I just need to know the places where you changed code, ignore formatting. For sure you had to edit some files to make the code work. We need to know what you had to modify.

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.

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:

  1. I created .github/workflows/build_and_test_debug_manager.yml
  2. I updated various paths and imports in the following:
server_manager/base.webpack.js
server_manager/electron_app/build.action.sh
server_manager/electron_app/package.action.sh
server_manager/electron_app/start.action.sh
server_manager/electron_main.webpack.mjs
server_manager/electron_preload.webpack.mjs
server_manager/web_app/build_install_script.action.sh
server_manager/web_app/build.action.sh
server_manager/web_app/outline-gcp-create-server-app.ts
server_manager/web_app/start.action.sh
server_manager/web_app/ui_components/outline-server-list.ts
  1. I updated dependencies and rules in these:
server_manager/package.json
server_manager/tsconfig.json

@daniellacosse daniellacosse changed the title proposal(manager): move the manager and rename the repository to outline_apps chore(manager): move the manager and rename the repository to outline_apps Feb 21, 2024
Copy link
Collaborator

@fortuna fortuna 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. 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 @@
{
Copy link
Collaborator

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.

@daniellacosse daniellacosse merged commit 81abbb9 into master Feb 21, 2024
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants