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: allow running review app to validate pr from external contribution #253

Conversation

FabienArcellier
Copy link
Collaborator

@FabienArcellier FabienArcellier commented Feb 19, 2024

The integration of an app review mechanism deploys a PR on demand to test the code online or demonstrate it without requiring cloning the branch locally.

Peek 2024-02-19 18-05

I used the reviewapp mechanism from scalingo, a French PaaS platform. Their implementation is close to Heroku. After a first reading, project administrators will be able to trigger an environment to try and experiment on the code of the PR.

Peek 2024-02-19 17-24

The validation environment can be modified by any visitor. If this mechanism poses a problem, we will add authentication to limit access and only offer read access for visitors.

* configure streamsync to be able to deploy review application
@FabienArcellier FabienArcellier added the enhancement New feature or request label Feb 19, 2024
@@ -0,0 +1,2 @@
https://github.com/FabienArcellier/nodejs-buildpack#streamsync-review
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know where to document the role of the Procfile and .buildpacks. .buildpacks does not support code comment.

* create a review app to access to different basic streamsync application
@FabienArcellier FabienArcellier force-pushed the 26-allow-running-review-app-to-validate-pr-from-external-contribution branch from 7ff972b to 3e0363a Compare February 19, 2024 19:25
@ramedina86
Copy link
Collaborator

The validation environment can be modified by any visitor. If this mechanism poses a problem, we will add authentication to limit access and only offer read access for visitors.

Is it too hard to implement auth with something like Google Auth and whitelist a few people?

Running streamsync run for everyone and streamsync edit for whitelisted would be ideal, but I'm sure harder to implement.

We'll be deploying many features that'll require us to handle API keys and maybe other sorts of confidential information. Being overly cautious but I'm scared someone could install a keylogger / etc, dump memory, or do something nasty.

* configure authentication layer on review application
@FabienArcellier
Copy link
Collaborator Author

FabienArcellier commented Feb 20, 2024

The validation environment can be modified by any visitor. If this mechanism poses a problem, we will add authentication to limit access and only offer read access for visitors.

Is it too hard to implement auth with something like Google Auth and whitelist a few people?

Running streamsync run for everyone and streamsync edit for whitelisted would be ideal, but I'm sure harder to implement.

We'll be deploying many features that'll require us to handle API keys and maybe other sorts of confidential information. Being overly cautious but I'm scared someone could install a keylogger / etc, dump memory, or do something nasty.

I added authentication when the review application listens for remote access. When the app is accessible in remote access, the basic auth configuration is mandatory. It is done using the BASICAUTH environment variable in validation environment. In the event of a connection failure, the connection module is blocked for 1 second, making the bruteforce approach impossible. The application becomes completely inaccessible to everyone during this time.

OAuth2 is easy with library. I will prepare an implementation to compare.

@@ -34,6 +34,7 @@ watchdog = ">= 3.0.0, < 4"
pandas = {version = ">= 2.2.0, < 3", optional = true}
pyarrow = {version = ">= 15.0.0, < 16.0.0",optional = true}
plotly = {version = ">= 5.18.0, < 6", optional = true}
scikit-learn = {version = "^1.4.1.post1", optional = true}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this here? Do we require it now for any reason?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are using it in the getting started application.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah you're right, and this doesn't bloat the standard distributions (bare, ds) right? Just the build

@ramedina86
Copy link
Collaborator

Hey @FabienArcellier what's the status of this? Are we going for the custom buildpack? If it's too hard we can even drop the quickstart app, I wouldn't spend too much time / add too much complexity to support this

@FabienArcellier
Copy link
Collaborator Author

I have finished the work on the buildpack. This branch is not required anymore. I close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants