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.

Sign up for GitHub

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

Jump to bottom

data: profile: extend workstation profile to hide pages for Web UI #6047

Merged
merged 2 commits into from
Jan 14, 2025
Merged

data: profile: extend workstation profile to hide pages for Web UI #6047

merged 2 commits into from
Jan 14, 2025

Conversation

KKoukiou
Copy link
Contributor

This was previously handled within the Web UI codebase. Now we hide pages specified by the hidden_pages configuration option.

Related to: rhinstaller/anaconda-webui#551
Related to: https://issues.redhat.com/browse/INSTALLER-4085

@github-actions github-actions bot added the f42 Fedora 42 label Dec 13, 2024
jkonecny12
jkonecny12 requested changes Dec 16, 2024
View reviewed changes
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.

You also need to extend anaconda.conf with the defaults of the field. Also please add comments.

I also wonder if it shouldn't be hidden_webui_pages to be crystal clear.

@KKoukiou
Copy link
Contributor Author

You also need to extend anaconda.conf with the defaults of the field. Also please add comments.

I also wonder if it shouldn't be hidden_webui_pages to be crystal clear.

I am in for the clarification.

@KKoukiou
Copy link
Contributor Author

You also need to extend anaconda.conf with the defaults of the field. Also please add comments.

I also wonder if it shouldn't be hidden_webui_pages to be crystal clear.

I am in for the clarified version.

@KKoukiou KKoukiou force-pushed the hidden_screens_webui branch from 1c56415 to 3d7771b Compare December 16, 2024 12:44
@KKoukiou KKoukiou force-pushed the hidden_screens_webui branch from 3d7771b to 9683d73 Compare December 16, 2024 12:49
M4rtinK
M4rtinK approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@M4rtinK M4rtinK 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. :)

Longer term we might need something more granular and/or dynamic, but this should work fine for now. :)

@KKoukiou
Copy link
Contributor Author

/kickstart-tests --waive webui config

@KKoukiou KKoukiou requested a review from jkonecny12 December 17, 2024 06:47
KKoukiou
KKoukiou commented Dec 17, 2024
View reviewed changes
Copy link
Contributor Author

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

So I was re-thinking about this. I dont think screen is the correct item to specify hiding. I think we should allow hiding components .
The issue is that the screens might incorportate multiple configurations, aka language together with keyboard and timezone.

Maybe we want to keep the language selection for Workstation, but we dont want the timezone.

I believe we need some more fine grained approach here.

What do you think? @jkonecny12 @M4rtinK

@jkonecny12
Copy link
Member

Sounds like a good approach. We just need to make it easy to list possible options. It could be comments in the configuration file?

@KKoukiou
Copy link
Contributor Author

Sounds like a good approach. We just need to make it easy to list possible options. It could be comments in the configuration file?

Sorry now that I am rethinking this, this would complicate things quite a lot code wise. So I suggest to ignore my comment, start by disabling wizard page level components, and and if in the future the design forces us to be more specific, I will re-think it.

KKoukiou added a commit to KKoukiou/anaconda-webui that referenced this pull request Dec 23, 2024
@KKoukiou
Parse new 'hidden_screen' configuration file options
4cff6c5 
Previously we would be using the ISO variant (Boos, Live) from Anaconda configuration
file and we would conditionalize the page visibility in the Web UI code
according to that.
With this commit, the pages can be hidden per-new option of the
configuration file [1].

[1] rhinstaller/anaconda#6047
@KKoukiou KKoukiou force-pushed the hidden_screens_webui branch from 9683d73 to a39a497 Compare January 2, 2025 14:05
@KKoukiou KKoukiou force-pushed the hidden_screens_webui branch from a39a497 to a48496d Compare January 2, 2025 14:13
KKoukiou added a commit to KKoukiou/anaconda-webui that referenced this pull request Jan 2, 2025
@KKoukiou
Parse new 'hidden_screen' configuration file options
23e934e 
Previously we would be using the ISO variant (Boos, Live) from Anaconda configuration
file and we would conditionalize the page visibility in the Web UI code
according to that.
With this commit, the pages can be hidden per-new option of the
configuration file [1].

[1] rhinstaller/anaconda#6047
KKoukiou added a commit to KKoukiou/anaconda-webui that referenced this pull request Jan 2, 2025
@KKoukiou
Parse new 'hidden_screen' configuration file options
5b232b0 
Previously we would be using the ISO variant (Boot, Live) from Anaconda configuration
file and we would conditionalize the page visibility in the Web UI code
according to that.
With this commit, the pages can be hidden per-new option of the
configuration file [1].

[1] rhinstaller/anaconda#6047
KKoukiou added a commit to KKoukiou/anaconda-webui that referenced this pull request Jan 3, 2025
@KKoukiou
Parse new 'hidden_screen' configuration file options
5747d7a 
Previously we would be using the ISO variant (Boot, Live) from Anaconda configuration
file and we would conditionalize the page visibility in the Web UI code
according to that.
With this commit, the pages can be hidden per-new option of the
configuration file [1].

[1] rhinstaller/anaconda#6047
@M4rtinK
Copy link
Contributor

M4rtinK commented Jan 6, 2025

Sorry now that I am rethinking this, this would complicate things quite a lot code wise. So I suggest to ignore my comment, start by disabling wizard page level components, and and if in the future the design forces us to be more specific, I will re-think it.

Yeah - while it would be ideal to work on the level of features, that also implies being able to map the features to pages & the resolve if the given page has all features it provides hidden. Far from simple I'm afraid + added complication due to inter page dependencies. (eq. storage page should go before software selection page, so it can check there ie enough space).

Working on page level is much more blunt bot certainly far simpler for now. :)

KKoukiou added a commit to KKoukiou/anaconda-webui that referenced this pull request Jan 7, 2025
@KKoukiou
Parse new 'hidden_screen' configuration file options
18022b1 
Previously we would be using the ISO variant (Boot, Live) from Anaconda configuration
file and we would conditionalize the page visibility in the Web UI code
according to that.
With this commit, the pages can be hidden per-new option of the
configuration file [1].

[1] rhinstaller/anaconda#6047
KKoukiou added a commit to KKoukiou/anaconda-webui that referenced this pull request Jan 8, 2025
@KKoukiou
Parse new 'hidden_screen' configuration file options
a6a8f03 
Previously we would be using the ISO variant (Boot, Live) from Anaconda configuration
file and we would conditionalize the page visibility in the Web UI code
according to that.
With this commit, the pages can be hidden per-new option of the
configuration file [1].

[1] rhinstaller/anaconda#6047
KKoukiou added a commit to KKoukiou/anaconda-webui that referenced this pull request Jan 8, 2025
@KKoukiou
Parse new 'hidden_screen' configuration file options
e281d27 
Previously we would be using the ISO variant (Boot, Live) from Anaconda configuration
file and we would conditionalize the page visibility in the Web UI code
according to that.
With this commit, the pages can be hidden per-new option of the
configuration file [1].

[1] rhinstaller/anaconda#6047
@KKoukiou KKoukiou force-pushed the hidden_screens_webui branch from a48496d to e756cfd Compare January 8, 2025 17:15
KKoukiou added a commit to KKoukiou/anaconda-webui that referenced this pull request Jan 9, 2025
@KKoukiou
Parse new 'hidden_screen' configuration file options
b4f9a33 
Previously we would be using the ISO variant (Boot, Live) from Anaconda configuration
file and we would conditionalize the page visibility in the Web UI code
according to that.
With this commit, the pages can be hidden per-new option of the
configuration file [1].

[1] rhinstaller/anaconda#6047
jkonecny12
jkonecny12 reviewed Jan 13, 2025
View reviewed changes
data/profile.d/fedora-workstation.conf
@@ -22,6 +22,9 @@ hidden_spokes =
NetworkSpoke
PasswordSpoke
UserSpoke
hidden_webui_pages =
anaconda-screen-accounts
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the prefix? Will it be a situation when it is not anaconda-screen prefix?

If yes, could you please document it somewhere (commit message for example)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been discussed more in the web UI PR rhinstaller/anaconda-webui#551 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I see, ok.

Copy link
Member

Choose a reason for hiding this comment

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

I would still like to see the reasoning as part of the commit if possible.

jkonecny12
jkonecny12 approved these changes Jan 13, 2025
View reviewed changes
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 good to me. Thanks!

KKoukiou added a commit to KKoukiou/anaconda-webui that referenced this pull request Jan 14, 2025
@KKoukiou
Parse new 'hidden_screen' configuration file options
ca604fe 
Previously we would be using the ISO variant (Boot, Live) from Anaconda configuration
file and we would conditionalize the page visibility in the Web UI code
according to that.
With this commit, the pages can be hidden per-new option of the
configuration file [1].

[1] rhinstaller/anaconda#6047
@KKoukiou
data: profile: extend workstation profile to hide pages for Web UI
868f521 
This was previously handled within the Web UI codebase. Now we hide
pages specified by the `hidden_webui_pages` configuration option.

The hidden_webui_pages configuration option accepts an array of strings
mapping to Web UI wizard screen selectors.

The are using the 'anaconda-screen-' prefix followed up by the screen
purpose. So 'anaconda-screen-accounts', 'anaconda-screen-language'.

Using the prefixes would make it easier to find all places where it is used (tests, codebase etc)
& possibly make it less likely to be broken by mistake.
@KKoukiou
docs: add release note for the hidden-webui-pages configuration option
9b54f34
@KKoukiou KKoukiou force-pushed the hidden_screens_webui branch from e756cfd to 9b54f34 Compare January 14, 2025 11:46
@KKoukiou
Copy link
Contributor Author

/kickstart-tests --waive web ui only

@KKoukiou KKoukiou merged commit c863a30 into rhinstaller:main Jan 14, 2025
18 of 19 checks passed
@KKoukiou KKoukiou deleted the hidden_screens_webui branch January 14, 2025 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkonecny12 jkonecny12 jkonecny12 approved these changes

@M4rtinK M4rtinK M4rtinK approved these changes

Assignees
No one assigned
Labels
f42 Fedora 42
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KKoukiou @jkonecny12 @M4rtinK