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

Enable to set callback_url via environment variables #5533

Merged
merged 8 commits into from
Jan 16, 2023

Conversation

mats16
Copy link
Contributor

@mats16 mats16 commented Dec 29, 2022

Motivation and context

The callback url is hardcoded, so I cannot run CVAT on Cloud platform.
This patch enable to replace these parameters via environment variables.

#5526

How has this been tested?

Run on AWS Fargate with Google auth.

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

@mats16 mats16 changed the title add env for callback_url Enable to set callback_url via environment variables Dec 29, 2022
nmanovic
nmanovic previously approved these changes Dec 29, 2022
Copy link
Contributor

@nmanovic nmanovic left a comment

Choose a reason for hiding this comment

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

LGTM

@nmanovic
Copy link
Contributor

@mats16 , thanks for your contribution!

@Marishka17
Copy link
Contributor

@nmanovic, @mats16, I guess we should use CVAT_HOST environment variable because auth/login-with-social-app , auth/<provider>/login/callback/ are part of our server routes and should be immutable.

@mats16
Copy link
Contributor Author

mats16 commented Dec 29, 2022

@nmanovic @Marishka17
I think it is better to use CVAT_HOST too. But CVAT_HOST does not have schema.

Can I add the environmental variable such as CVAT_EXTERNAL_URL and use it instead of CVAT_HOST?

@nmanovic
Copy link
Contributor

@nmanovic @Marishka17 I think it is better to use CVAT_HOST too. But CVAT_HOST does not have schema.

Can I add the environmental variable such as CVAT_EXTERNAL_URL and use it instead of CVAT_HOST?

@mats16 , there is one more opportunity. You can import our settings file into your my_settings.py and redefine what even you want. Will it work for you?

@nmanovic nmanovic dismissed their stale review December 30, 2022 11:50

See comments in the PR

@mats16
Copy link
Contributor Author

mats16 commented Dec 30, 2022

@nmanovic

there is one more opportunity. You can import our settings file into your my_settings.py and redefine what even you want. Will it work for you?

I feel it is not good option... Because CVAT project provide container images officially, it is better to configure via environment variables. I want use official container images.

If you need more time to discuss about CVAT_HOST, it is better to release once asGOOGLE_CALLBACK_URL.
CALLBACK_URL: localhost:8080 obviously does not work.

@Marishka17
Copy link
Contributor

@mats16, Why is the approach of using CVAT_HOST + another environment variable (e.g CVAT_SCHEME or USE_HTTPS) worse than adding GITHUB_CALLBACK_URL, GOOGLE_CALLBACK_URL, SOCIAL_APP_LOGIN_REDIRECT_URL?
We can add the scheme env variable in docker-compose.https.yml and use it to define the scheme in our settings (default http).

@mats16
Copy link
Contributor Author

mats16 commented Jan 4, 2023

@Marishka17
I don't think adding other environment variables (e.g CVAT_SCHEME or USE_HTTPS) is so bad. This is just my personal opinion...

  • When publishing a web application over the internet, a schema (such as https) is required and it is almost never necessary to use only hostname.
  • If you want to host CAVT under a specific url path (such as https://example.com/cvat/), you cannot configure via CVAT_SCHEMA and CVAT_HOST environment variables. You need another variables.

How about the following code?

CVAT_HOST = os.getenv('CVAT_HOST', 'localhost')

CVAT_BASE_URL = os.getenv('CVAT_BASE_URL', f'http://{CVAT_HOST}:8080').rstrip('/')

SOCIAL_APP_LOGIN_REDIRECT_URL = f'{CVAT_BASE_URL}/auth/login-with-social-app'
GITHUB_CALLBACK_URL = f'{CVAT_BASE_URL}/api/auth/github/login/callback/'
GOOGLE_CALLBACK_URL = f'{CVAT_BASE_URL}/api/auth/google/login/callback/'

@Marishka17
Copy link
Contributor

@mats16, Hi, sorry for the long response. Okay, it looks good to me.

cvat/settings/base.py Outdated Show resolved Hide resolved
@nmanovic
Copy link
Contributor

@mats16 ,

could you please remove trailing spaces? Pylint isn't happy.

@mats16
Copy link
Contributor Author

mats16 commented Jan 13, 2023

Sorry, I fixed it.

@nmanovic
Copy link
Contributor

@mats16 , thanks for the contribution!

@nmanovic nmanovic merged commit 8f71d90 into cvat-ai:develop Jan 16, 2023
mikhail-treskin pushed a commit to retailnext/cvat that referenced this pull request Jul 1, 2023
The callback url is hardcoded, so I cannot  run CVAT on Cloud platform.
This patch enable to replace these parameters via environment variables.

cvat-ai#5526
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.

3 participants