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 #38019 - Add hammer support for flatpak remotes and remote repositories #971

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

sjha4
Copy link
Member

@sjha4 sjha4 commented Nov 19, 2024

Adds hammer support for flatpak remotes. The sub-commands added are list/show/create/update/delete and scan.
This also supports list/info on remote repository and adds a command to mirror remote repository into katello.

This requires Katello/katello#11223 for testing..

Working on more tests but the commands should be ready for testing.

@sjha4 sjha4 force-pushed the flatpak_hammer branch 5 times, most recently from 9d23163 to d560b36 Compare November 22, 2024 14:56
@sjha4 sjha4 changed the title Fixes #38019 - Add hammer support for flatpak remotes Fixes #38019 - Add hammer support for flatpak remotes and remote repositories Nov 22, 2024
@sjha4
Copy link
Member Author

sjha4 commented Dec 4, 2024

Test failures are due to rubocop autocorrect on my last push..Will push..

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

It seems there are missing organization options for deleting remotes:

Usage:
    hammer flatpak-remote <delete|destroy> [OPTIONS]

Options:
 --id NUMBER                   Flatpak remote id
 --name VALUE                  Flatpak remote name to search by
 -h, --help                    Print help

[vagrant@centos7-hammer-devel hammer-cli-katello]$ hammer flatpak-remote delete --name "Fedora flatpak"
Could not delete the Flatpak Remote.:
  Error: Missing options to search organization.

[vagrant@centos7-hammer-devel hammer-cli-katello]$ hammer flatpak-remote delete --name "Fedora flatpak" --organization-id 1
Could not delete the Flatpak Remote.:
  Error: Unrecognised option '--organization-id'.

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

The help text for creating remotes is a little confusing, specifically for the URLs:

Usage:
    hammer flatpak-remote create [OPTIONS]

Options:
 --description VALUE                            Description of the flatpak remote
 --name VALUE                                   Name
 --organization[-id|-title|-label] VALUE/NUMBER Organization name/title/label/id
 --registry-url VALUE                           Optional Registry url. Will be updated when remote is scanned.
 --token VALUE                                  Token/password for the flatpak remote
 --url VALUE                                    Url
 --username VALUE                               Username for the flatpak remote
 -h, --help                                     Print help

It isn't quite clear what the difference is between the url and the registry-url. It also isn't clear why someone would need to enter a registry-url.

@ianballou
Copy link
Member

Besides my comments above, the general workflows seem to be working well 👍

@sjha4 sjha4 force-pushed the flatpak_hammer branch 2 times, most recently from 34bf5cc to 980bb2b Compare December 6, 2024 16:48
@sjha4
Copy link
Member Author

sjha4 commented Dec 6, 2024

Updated to address reviews. 👍🏼

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

Some more comments:

lib/hammer_cli_katello/flatpak_remote.rb Show resolved Hide resolved
lib/hammer_cli_katello/flatpak_remote.rb Outdated Show resolved Hide resolved
test/functional/flatpak_remote/list_test.rb Outdated Show resolved Hide resolved
end
end

class MirrorCommand < HammerCLIKatello::SingleResourceCommand
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps unrelated to this PR, but do you know what "organization title" is?

We typically only have ID / Name / Label...

Usage:
    hammer flatpak-remote remote-repository mirror [OPTIONS]

Options:
 --async                                        Do not wait for the task
 --flatpak-remote[-id] VALUE/NUMBER             Name/Id of associated flatpak remote
 --id NUMBER                                    Flatpak remote repository id
 --name VALUE                                   Name to search by
 --organization[-id|-title|-label] VALUE/NUMBER Name/Title/Label/Id of associated organization
 --product[-id] VALUE/NUMBER                    Product name/id to mirror the remote repository to
 -h, --help                                     Print help

Copy link
Member Author

Choose a reason for hiding this comment

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

[2] pry(main)> org.title
=> "Default Organization"
[3] pry(main)> org.name
=> "Default Organization"
[4] pry(main)> org.label
=> "Default_Organization"

Title seems to be the same as name.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting!

@sjha4 sjha4 force-pushed the flatpak_hammer branch 2 times, most recently from 0cf5a73 to 2c53124 Compare December 9, 2024 14:23
Copy link
Member

@ianballou ianballou 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!

Copy link

@vsedmik vsedmik 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.

@sjha4
Copy link
Member Author

sjha4 commented Dec 11, 2024

Merging.. 🎉

@sjha4 sjha4 merged commit 4eab7ae into Katello:main Dec 11, 2024
11 checks passed
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