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

localization: allow to turn off geolocation by configuration #4719

Merged
merged 1 commit into from May 9, 2023

Conversation

rvykydal
Copy link
Contributor

@rvykydal rvykydal commented Apr 28, 2023

New Localization section with use_geolocation boolean option is added to
Anaconda configuration.

Intended use: On Fedora Workstation (Gnome) the language will be set by
environment initial setup and Anaconda will take over this value.
This option set to False in the Workstation configuration profile would
prevent overriding the value from environment with geolocation.

INSTALLER-3472

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks great to me. Thanks!

Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!

Looking at the description of order of precedence, I wonder if this could use a similar mechanism to Timezone - multiple inputs with varying priorities. But the scope still seems to be manageable as is.

@jkonecny12
Copy link
Member

I wonder why the rpm-tests are failing? npm issue?

Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

This doesn't follow the documentation nor the conventions.

configuration drop directory (/etc/anaconda/conf.d), eg Gnome Inital Setup.

Priority of the configuration:
inst.lang > kickstart lang > conf.localization.lang > geolocation
Copy link
Contributor

@poncovka poncovka Apr 28, 2023

Choose a reason for hiding this comment

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

This is stressing me out. The configuration file always has the lowest priority, because it provides defaults. It is documented here: https://anaconda-installer.readthedocs.io/en/latest/configuration-files.html#runtime-configuration-file So it cannot be never ever higher than a kickstart file or boot options. I strongly recommend to follow this practice. You already struggle with mixed priorities of kickstart commands and boot options.

I looked up the Jira issue and I don't fully understand the use cases you are trying to cover. Don't you target Fedora Workstation? Why are kickstart and boot options dragged into this?

The comment at https://github.com/rhinstaller/anaconda-webui/issues/1#issuecomment-1516406850 says that the pre-selected language will be applied to the Live environment. Anaconda should be actually able to pick up this value automatically if the language is not set (see the code below). That means you are probably already able to handle your use case without any further changes in the code.

# Look for a locale in the environment. If the variable is setup but
# empty it doesn't count, and some programs (KDE) actually do this.
# If prefer_environment is set, the environment locale can override
# the parameter passed in. This can be used, for example, by initial-setup,
# to prefer the possibly-more-recent environment settings before falling back
# to a locale set at install time and saved in the kickstart.
if not locale or prefer_environment:
for varname in ("LANGUAGE", "LC_ALL", "LC_MESSAGES", "LANG"):
if varname in os.environ and os.environ[varname]:
locale = os.environ[varname]

Btw. the language seems to be a good candidate for introducing someting like SetLanguageWithPriority (see SetTimezoneWithPriority) and get rid of LanguageKickstarted, but that is probably out of scope of this issue.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting idea. I missed the possibility to read the LANGUAGE value from the system but it definitely give sense. So we might be able to avoid that.

Instead of introducing a new configuration option we could just read the value from the system and disable geolocation in Live environemnts? However, the issue then is, how we would know that the value is changed by Gnome Initial Setup or just default of the system (then we want user to choose or detect language by geolocation). Any ideas? I would like to avoid detection of GIS did something and the other option leads me again to configuration option.

Maybe we can change the configuration option. Instead of pre-selected language the GIS can just disable geolocation?

Copy link
Contributor Author

@rvykydal rvykydal Apr 28, 2023

Choose a reason for hiding this comment

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

This is stressing me out. The configuration file always has the lowest priority, because it provides defaults. It is documented here: https://anaconda-installer.readthedocs.io/en/latest/configuration-files.html#runtime-configuration-file So it cannot be never ever higher than a kickstart file or boot options. I strongly recommend to follow this practice. You already struggle with mixed priorities of kickstart commands and boot options.

I am not following, the priority scheme wanted to say that the priority is the same as before with the configuration option just overriding the default or geolocation.
But it really seems we are misusing configuration to do something it should not to (that is why I asked Vendy for review). We should then rather turn off geolocation for our use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not following, the priority scheme wanted to say that the priority is the same as before with the configuration option just overriding the default or geolocation.

Right, I have got confused by the order. My mistake. Still, it doesn't make sense to prioritize the configuration option over the geolocation result.

We should then rather turn off geolocation for our use case.

Agreed.

@rvykydal rvykydal added the blocked Don't merge this pull request! label May 3, 2023
@rvykydal rvykydal changed the title localization: add Localization section and lang command to configuration localization: allow to turn off geolocation by configuration May 5, 2023
…ration

New Localization section with use_geolocation boolean option is added to
Anaconda configuration.

Intended use: On Fedora Workstation (Gnome) the language will be set by
environment initial setup and Anaconda will take over this value.
This option set to False in the Workstation configuration profile would
prevent overriding the value from environment with geolocation.

INSTALLER-3472
@rvykydal
Copy link
Contributor Author

rvykydal commented May 5, 2023

/kickstart-test --testtype smoke

Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks great to me.

Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!

@rvykydal rvykydal merged commit 682b3f6 into rhinstaller:master May 9, 2023
14 of 16 checks passed
@rvykydal rvykydal removed the blocked Don't merge this pull request! label May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 participants