Navigation Menu

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

Fixes #31177 - make registration variables more readable #8106

Merged
merged 1 commit into from Nov 12, 2020

Conversation

ares
Copy link
Member

@ares ares commented Oct 26, 2020

No description provided.

@theforeman-bot
Copy link
Member

Issues: #31177

@@ -19,18 +19,20 @@ def global_registration_vars
host_group = Hostgroup.authorized(:view_hostgroups).find(params['hostgroup_id']) if params["hostgroup_id"].present?
operatingsystem = Operatingsystem.authorized(:view_operatingsystems).find(params['operatingsystem_id']) if params["operatingsystem_id"].present?

context = {
user: User.current,
auth_token: User.current.jwt_token!(expiration: 4.hours.to_i),
Copy link
Member

Choose a reason for hiding this comment

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

Any idea why we are using 4.hours.to_i, does 4 hours has a significance or it is a random time frame?

Copy link
Member

Choose a reason for hiding this comment

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

It was there before, just moving things around. However, I would agree that it's ugly that it's hardcoded and not even a constant.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

a bit of offtopic to this PR: We could extract that to be configurable, however is that necessary? Personally I'd wait until someone asks for it, the first token is configurable already. We have enough settings already. The other option would be to base on the expiration set for the first request. However do not that these two expirations are not the same thing. If you think it's valuable, please describe how we should set this and link the issue to https://projects.theforeman.org/issues/30440.

@theforeman-bot
Copy link
Member

@ares, this pull request is currently not mergeable. Please rebase against the develop branch and push again.

If you have a remote called 'upstream' that points to this repository, you can do this by running:

    $ git pull --rebase upstream develop

This message was auto-generated by Foreman's prprocessor

@ares
Copy link
Member Author

ares commented Nov 11, 2020

PR rebased and empty line added, this should be good for merge

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Thanks!

@ekohl ekohl merged commit 53d5bc8 into theforeman:develop Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants