-
Notifications
You must be signed in to change notification settings - Fork 786
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
Next js prototype #3896
Next js prototype #3896
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3896 +/- ##
===========================================
- Coverage 78.01% 77.57% -0.44%
===========================================
Files 464 467 +3
Lines 14672 14759 +87
===========================================
+ Hits 11447 11450 +3
- Misses 3225 3309 +84 ☔ View full report in Codecov by Sentry. |
key={name} | ||
data-emotion={`${registry.cache.key}-global ${name}`} | ||
// eslint-disable-next-line react/no-danger | ||
dangerouslySetInnerHTML={{ __html: style }} |
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 temporary? We shouldn't use dangerouslySetInnerHTML
because well, the name says 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.
@ordabach do you know where the styles are coming from?
63537b6
to
2aa2396
Compare
4fb8aa7
to
f2a021b
Compare
// !! WARN !! | ||
// Dangerously allow production builds to successfully complete even if | ||
// your project has type errors. | ||
// !! WARN !! | ||
ignoreBuildErrors: true |
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.
Why do we allow this?
return null; // clear the session if the refresh token gets an error | ||
|
||
// The error property will be used client-side to handle the refresh token error | ||
// return { | ||
// ...token, | ||
// error: 'RefreshAccessTokenError' as const | ||
// }; |
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.
Are we going to do this later?
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 whole authentication needs a lot of work
); | ||
|
||
const logoutResponseBody = await logoutResponse.json(); | ||
console.log('logoutResponseBody', logoutResponseBody); |
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.
Do we need this?
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 can keep it, I'll have to review the authentication anyway and log statements are ignored in production builds
); | ||
|
||
const registerResponseBody = await registerResponse.json(); | ||
console.log('registerResponseBody', registerResponseBody); |
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.
Same as above.
// TODO remove with the NEXT_JS_UI_FEATURE in monkey_island/cc/feature_flags.py:1 | ||
return process.env.NEXT_PUBLIC_JAVASCRIPT_RUNTIME_PORT || 5000; |
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.
Can we do this now?
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.
Not yet, we'll do it once the old UI is removed
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.
A few comments. I didn't review everything since I needed to learn about Redux and NextAuth
### Added | ||
- Configurable Island port through node proxy server. #3827 |
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'll need to devise a way to expose this port through docker. Either that or provide documentation on how to expose the configured port when running the container
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.
AFAIK --network=host
already exposes the port.
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.
That may be correct. I tried to test out this feature in docker and couldn't access the UI. I realized that modifying the server port is only applicable to the old UI but I still couldn't get it to work. In fact, if I modify it, the UI is not available at the original port (5000) or the new one
@@ -235,7 +239,7 @@ pushd "$ISLAND_PATH/cc/ui" || handle_error | |||
npm ci | |||
|
|||
log_message "Generating front end" | |||
npm run dev | |||
npm run build |
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.
Does the current UI support a build
target? We used to run dev
or dist
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'm not sure what value the dev
build had, but on the new UI we can build production or run a dev server with hot reloads. This builds production in case the dev is not intending to work on UI.
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.
Let me rephrase. This is generating the frontend for the old UI ("$ISLAND_PATH/cc/ui"
). Does build
work for it?
... | ||
} | ||
``` | ||
1. Run the Monkey Island again, it will be accessible on `https://localhost:8080`. |
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.
Do we also need to tell docker to expose the new port?
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 don't think so, but I haven't tested it IIRC
{{% notice info %}} | ||
You need to have **root permissions** to run the Infection Monkey AppImage package. | ||
By default, the Infection Monkey server is configured to listen on **port 443**, | ||
which requires setting the `CAP_NET_BIND_SERVICE` capability on the AppImage package. |
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 TMI? Should we just say that serving on port 443 requires root permission?
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.
What I was trying to say is that the process will not run as root, but root permissions are required. Feel free to rephrase or shorten
The script is not necessary, because production is started by island anyway
Moving prettier config to a separate file makes more readable and allows other tools to find it
b379f1b
to
db2e4d2
Compare
What does this PR do?
Fixes #3793
Fixes #3794
Fixes #2898
PR adds next.js server and UI prototype using a feature flag.
PR Checklist
Testing Checklist