Skip to content

Conversation

@tardyp
Copy link
Contributor

@tardyp tardyp commented Mar 26, 2021

when deploying this outside of heroku, we get strange 400 errors, which turn out to be this variable that needs to be configured.

@Hritik14
Copy link
Collaborator

This is strange. Why do we even have heroku hardcoded in there?

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 29, 2021

@Hritik14
There was a demo thing up after gsoc 2017 on heroku, hence it stayed there I guess.

@sbs2001 sbs2001 requested a review from tdruez March 29, 2021 03:45
@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 29, 2021

@tardyp thanks for the PR ! I'm passing up this to @tdruez to review.

@Hritik14
Copy link
Collaborator

@sbs2001 I guess we should remove heroku as well. It could turn out to really unwanted for some installations.


ALLOWED_HOSTS = [
".herokuapp.com",
os.environ.get("VC_ALLOWED_HOSTS", ".herokuapp.com"),
Copy link
Contributor

Choose a reason for hiding this comment

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

VC_ALLOWED_HOSTS here is not great since you are providing a single host through the environ and not a list of hosts.
I would suggest a simple VC_HOST for this env variable.
Also, we probably want to remove the ".herokuapp.com" default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is better to keep it near the django semantics, so I switched the variable to be splitable.

I removed herokuapp to switch to '*' for the default, as I am not sure we really want to get it not work by default.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that VC_ALLOWED_HOSTS can be used to specify multiple hosts separated via a colon, it solves both of the issues you guys pointed out ?

Signed-off-by: Pierre Tardy <pierre.tardy@renault.com>
@Hritik14
Copy link
Collaborator

Is it not better to let the let the user configure their settings.py by themselves ?
We could document it further and arrange it better and point the users to configure their settings.py in the readme.

@tardyp
Copy link
Contributor Author

tardyp commented Mar 29, 2021

In my case, I am deploying vulnerablecode via docker in a kubernetes cluster, so I would prefer to have a default configuration which does not require to modify the app code.

@sbs2001 sbs2001 requested a review from tdruez March 30, 2021 04:23
@sbs2001 sbs2001 merged commit dfb1ab5 into aboutcode-org:main Mar 31, 2021
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.

4 participants