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

Move hardcoded flatpak remote to configuration #5493

Merged

Conversation

jkonecny12
Copy link
Member

@jkonecny12 jkonecny12 commented Feb 26, 2024

Without this change Anaconda always set the remote after flatpak
deployment to

name: fedora
remote: oci+https://registry.fedoraproject.org/

that worked until now. However, uBlue[1] want's to use Flathub registry as the remote source.

To enable this we should move the flatpak remote from code to configuration files and they should create this configuration file during the ISO creation.

Another use-case for this are ISO images for container based OSTree deployments. The tool to create an ISO could also enable user to set their own set of flatpaks from any remote source to the ISO image with installer for offline deployment and changing this configuration to set the correct remote after installation.

Please check that your PR follows these rules:

  • Code conventions. tl;dr: Follow PEP8, wrap at 100 chars, and provide docstrings.

  • Commit message conventions. tl;dr: Heading, empty line, longer explanations, all wrapped manually. If in doubt, write a longer commit message with more details.

  • Tests pass and cover your changes. You can run tests locally manually, or have them run as part of the PR. A team member will have to enable tests manually after inspecting the PR.
    If you don't know how, ask us for help!

  • Release notes and docs if the PR adds something major or changes existing behavior.
    If you don't know how, ask us for help!

Note: This is community contribution in my free time - not a team effort 😁

@jkonecny12 jkonecny12 added blocked Don't merge this pull request! release note required Write a release note for this change. and removed documentation labels Feb 26, 2024
@jkonecny12
Copy link
Member Author

I'm setting blocked because I'm not able to find an ISO for SIlverblue so I'm not able to test this.

Copy link
Contributor

@travier travier left a comment

Choose a reason for hiding this comment

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

Minor suggestions and a comment 👍🏻

these Flatpaks are deployed and the remote was hardcoded to remote Fedora. This remote
is then used for updating the Flatpaks after installation.

After this change Flatpak remote could be set by ``flatpak_remote`` key in the configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
After this change Flatpak remote could be set by ``flatpak_remote`` key in the configuration
After this change Flatpak remote can be set by ``flatpak_remote`` key in the configuration

@@ -105,6 +105,13 @@ enable_closest_mirror = True
#
default_source = CLOSEST_MIRROR

# Default remote source after deployment of local Flatpaks from the ISO.
# Only one value could be set.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Only one value could be set.
# Only one value can be set.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can allow more than one value here? It is valid to have multiple remotes enabled. But then of course we have the question about which one is the default one.

def flatpak_remote(self):
"""Default remote source after deployment of local Flatpaks from the ISO.

Only one value could be set.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Only one value could be set.
Only one value can be set.

@travier
Copy link
Contributor

travier commented Feb 26, 2024

You can use https://pagure.io/workstation-ostree-config/blob/main/f/justfile#_211 to build an ISO. You need to build a classic ostree commit before:

$ just compose-legacy silverblue
$ just lorax silverblue

@travier
Copy link
Contributor

travier commented Feb 26, 2024

You can also download a nightly compose from https://openqa.fedoraproject.org/nightlies.html

@jkonecny12 jkonecny12 force-pushed the master-move-flatpak-remote-to-config branch from 86d6d0a to 173bb4e Compare February 28, 2024 21:42
@jkonecny12
Copy link
Member Author

@travier thanks for review - resolved

Also tested on latest Rawhide Silverblue image and it works good. This is ready for review and merge.

@jkonecny12
Copy link
Member Author

/kickstart-test --testtype smoke

@jkonecny12 jkonecny12 removed the blocked Don't merge this pull request! label Feb 28, 2024
@jkonecny12 jkonecny12 force-pushed the master-move-flatpak-remote-to-config branch from 173bb4e to d7b37b9 Compare March 4, 2024 10:17
@jkonecny12
Copy link
Member Author

/kickstart-test --testtype smoke

with pytest.raises(ValueError):
convert("test remote URL")

with pytest.raises(ValueError):
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont understand what this second exception checking is offering compared to the one one line above.

Would be worthy to add some comment for the different test cases, so that people who are not that familiar (see me) can understand that faster.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense now - thanks :)

Without this change Anaconda always set the remote after flatpak
deployment to

name: fedora
remote: oci+https://registry.fedoraproject.org

that worked until now. However, uBlue[1] want's to use Flathub registry
as the remote source.

To enable this we should move the flatpak remote from code to
configuration files and they should create this configuration file
during the ISO creation.

Another use-case for this are ISO images for container based
OSTree deployments. The tool to create an ISO could also
enable user to set their own set of flatpaks from any remote source to
the ISO image with installer for offline deployment and changing this
configuration to set the correct remote after installation.

[1]: https://universal-blue.org/
@jkonecny12 jkonecny12 force-pushed the master-move-flatpak-remote-to-config branch from d7b37b9 to 23dae6f Compare March 11, 2024 14:01
@jkonecny12
Copy link
Member Author

  • Rebased
  • Added one more sub test
  • Add comments to tests for better explanation.

@jkonecny12
Copy link
Member Author

/kickstart-test --testtype smoke

Copy link
Contributor

@travier travier left a comment

Choose a reason for hiding this comment

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

LGTM

@jkonecny12
Copy link
Member Author

Failed check is not related. Merging.

@jkonecny12 jkonecny12 merged commit 37eea2c into rhinstaller:master Mar 11, 2024
16 of 17 checks passed
@noelmiller
Copy link

I'm so happy this got merged. Will this feature be available in versions of Anaconda earlier than Fedora 41? We found a work around for creating our installer, but it would be awfully nice to remove that work around if we can just add a config option instead!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f41 release note required Write a release note for this change.
Development

Successfully merging this pull request may close these issues.

4 participants