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

3964 docker feature flags #3966

Merged
merged 1 commit into from
Dec 19, 2023
Merged

Conversation

ilija-lazoroski
Copy link
Contributor

What does this PR do?

Fixes #3964

Running of docker with feature flags:

sudo docker run \   
    --tty \
    --interactive \
    --name monkey-island \
    --network=host \
    --env FEATURE_FLAGS="NEXT_JS_UI" \
    infectionmonkey/monkey-island:614b9f658

Or for multiple environments check this

PR Checklist

  • Have you added an explanation of what your changes do and why you'd like to include them?
  • Is the TravisCI build passing?
  • Was the CHANGELOG.md updated to reflect the changes?
  • Was the documentation framework updated to reflect the changes?
  • Have you checked that you haven't introduced any duplicate code?

Testing Checklist

  • Added relevant unit tests?
  • Do all unit tests pass?
  • Do all end-to-end tests pass?
  • Any other testing performed?

    Tested by {Running the Monkey locally with relevant config/running Island/...}

  • If applicable, add screenshots or log transcripts of the feature working

@ilija-lazoroski ilija-lazoroski changed the base branch from develop to next-js-prototype December 19, 2023 12:12
Copy link

codecov bot commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2e4a246) 77.56% compared to head (614b9f6) 77.90%.

❗ Current head 614b9f6 differs from pull request most recent head 917d53b. Consider uploading reports for the commit 917d53b to get more accurate results

Additional details and impacted files
@@                  Coverage Diff                  @@
##           next-js-prototype    #3966      +/-   ##
=====================================================
+ Coverage              77.56%   77.90%   +0.33%     
=====================================================
  Files                    467      467              
  Lines                  14751    14712      -39     
=====================================================
+ Hits                   11442    11461      +19     
+ Misses                  3309     3251      -58     

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

@ilija-lazoroski ilija-lazoroski force-pushed the 3964-docker-feature-flags branch 2 times, most recently from 0562c8d to 87d9758 Compare December 19, 2023 13:11
ilija-lazoroski added a commit that referenced this pull request Dec 19, 2023
@cakekoa
Copy link
Contributor

cakekoa commented Dec 19, 2023

The build script will either build the original UI or the NextJS UI, correct? And if the NextJS UI is built, then one is required to run the docker container by passing in FEATURE_FLAGS environment variable? Can we do it in a way that wouldn't require the user to pass the env variable, since the feature is set at build time?

@ilija-lazoroski
Copy link
Contributor Author

The build script will either build the original UI or the NextJS UI, correct? And if the NextJS UI is built, then one is required to run the docker container by passing in FEATURE_FLAGS environment variable? Can we do it in a way that wouldn't require the user to pass the env variable, since the feature is set at build time?

I talked with @VakarisZ about this and we want to toggle between these two UI's. If we pass the FEATURE_FLAGS to build the NextJS then we probably want to toggle between the two UI's, if not then we don't care.

@cakekoa
Copy link
Contributor

cakekoa commented Dec 19, 2023

Also, do any additional ports need to be exposed in the docker container? I noticed we expose 5000 in the dockerfile, but the new UI uses 443

@ilija-lazoroski
Copy link
Contributor Author

Also, do any additional ports need to be exposed in the docker container? I noticed we expose 5000 in the dockerfile, but the new UI uses 443

We expose 443 as well:
image

@cakekoa
Copy link
Contributor

cakekoa commented Dec 19, 2023

The build script will either build the original UI or the NextJS UI, correct? And if the NextJS UI is built, then one is required to run the docker container by passing in FEATURE_FLAGS environment variable? Can we do it in a way that wouldn't require the user to pass the env variable, since the feature is set at build time?

I talked with @VakarisZ about this and we want to toggle between these two UI's. If we pass the FEATURE_FLAGS to build the NextJS then we probably want to toggle between the two UI's, if not then we don't care.

Wouldn't we need to build both then? Aren't we only building one currently?

@cakekoa
Copy link
Contributor

cakekoa commented Dec 19, 2023

Also, do any additional ports need to be exposed in the docker container? I noticed we expose 5000 in the dockerfile, but the new UI uses 443

We expose 443 as well: image

Okay, great. I didn't see it in the diff

@mssalvatore
Copy link
Collaborator

The build script will either build the original UI or the NextJS UI, correct? And if the NextJS UI is built, then one is required to run the docker container by passing in FEATURE_FLAGS environment variable? Can we do it in a way that wouldn't require the user to pass the env variable, since the feature is set at build time?

I talked with @VakarisZ about this and we want to toggle between these two UI's. If we pass the FEATURE_FLAGS to build the NextJS then we probably want to toggle between the two UI's, if not then we don't care.

I'm not sure I follow. As written, this will build one UI or the other. You can switch them at build time, but you wouldn't be able to toggle between them at runtime.

@VakarisZ VakarisZ merged commit 8be3860 into next-js-prototype Dec 19, 2023
@VakarisZ VakarisZ deleted the 3964-docker-feature-flags branch December 19, 2023 14:13
VakarisZ pushed a commit that referenced this pull request Dec 19, 2023
VakarisZ pushed a commit that referenced this pull request Dec 22, 2023
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.

Add feature flags to docker
4 participants