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

New command to list the custom packages #466

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

cmoulliard
Copy link
Contributor

@cmoulliard
Copy link
Contributor Author

cmoulliard commented Dec 24, 2024

Example of what the code produce now

Custom packages added

❯ idp create --color --recreate --name dummy --port 9443 \
   -p https://github.com/cnoe-io/stacks//basic/package1 \
   -p https://github.com/cnoe-io/stacks//basic/package2

What the command idp get packages prints

// A custom package
❯ ./idpbuilder get packages -p app-my-app
CUSTOM PACKAGE NAME   IDP NAMESPACE      GIT REPOSITORY                                                                      STATUS
app-my-app            idpbuilder-dummy   https://gitea.cnoe.localtest.me:9443/giteaAdmin/idpbuilder-dummy-my-app-manifests   true

// All the custom packages
~/code/cnoe/fork-idpbuilder on list-packages •
❯ ./idpbuilder get packages
CUSTOM PACKAGE NAME   IDP NAMESPACE      GIT REPOSITORY                                                                      STATUS
app-guestbook         idpbuilder-dummy   https://github.com/cnoe-io/stacks/tree/main/basic/package2                          true
app-my-app            idpbuilder-dummy   https://gitea.cnoe.localtest.me:9443/giteaAdmin/idpbuilder-dummy-my-app-manifests   true
app2-guestbook2       idpbuilder-dummy   https://github.com/cnoe-io/stacks/tree/main/basic/package2                          true

~/code/cnoe/fork-idpbuilder on list-packages •

Copy link
Collaborator

@nabuskey nabuskey left a comment

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we merge all structs in separate files into one file? Is the entity package for types? If so, we should name the package as types instead of entity.

Copy link
Contributor Author

@cmoulliard cmoulliard Jan 2, 2025

Choose a reason for hiding this comment

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

If so, we should name the package as types instead of entity.

types is a better name. I will rename. Fixed: 6415f7b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we merge all structs in separate files into one file?

Why do you want to merge them into one file ? This is easier to figure out what it exists if you have one file/type under the folder name types

pkg/cmd/get/packages.go Outdated Show resolved Hide resolved
pkg/cmd/get/packages.go Outdated Show resolved Hide resolved
@cmoulliard
Copy link
Contributor Author

Can we add additional printer columns for the package type?

Yes but what do you want to print under this new column ?

@cmoulliard
Copy link
Contributor Author

cmoulliard commented Jan 2, 2025

Question: Should we also add the column of the Argocd repo as we also have those for git or gitea repo ?
See: 0cd0d7d

❯ ./idpbuilder get packages
CUSTOM PACKAGE NAME   IDP NAMESPACE      GIT REPOSITORY                                                                      ARGOCD REPOSITORY                                                      STATUS
app-guestbook         idpbuilder-dummy   https://github.com/cnoe-io/stacks/tree/main/basic/package2                          https://argocd.cnoe.localtest.me:9443/applications/argocd/guestbook    true
app-my-app            idpbuilder-dummy   https://gitea.cnoe.localtest.me:9443/giteaAdmin/idpbuilder-dummy-my-app-manifests   https://argocd.cnoe.localtest.me:9443/applications/argocd/my-app       true
app2-guestbook2       idpbuilder-dummy   https://github.com/cnoe-io/stacks/tree/main/basic/package2                          https://argocd.cnoe.localtest.me:9443/applications/argocd/guestbook2   true

Signed-off-by: cmoulliard <[email protected]>
Copy link
Collaborator

@nabuskey nabuskey left a comment

Choose a reason for hiding this comment

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

Let's merge all files under the types directory. I don't think we should have a file for each struct. It's very Java like right now.😅

func generatePackageTable(packagesTable []types.Package) metav1.Table {
table := &metav1.Table{}
table.ColumnDefinitions = []metav1.TableColumnDefinition{
{Name: "Custom package name", Type: "string"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I liked "name" more. Custom package is already implied when you run the command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: 9bbd4d5

@nabuskey
Copy link
Collaborator

nabuskey commented Jan 2, 2025

Question: Should we also add the column of the Argocd repo as we also have those for git or gitea repo ?

See: 0cd0d7d


❯ ./idpbuilder get packages

CUSTOM PACKAGE NAME   IDP NAMESPACE      GIT REPOSITORY                                                                      ARGOCD REPOSITORY                                                      STATUS

app-guestbook         idpbuilder-dummy   https://github.com/cnoe-io/stacks/tree/main/basic/package2                          https://argocd.cnoe.localtest.me:9443/applications/argocd/guestbook    true

app-my-app            idpbuilder-dummy   https://gitea.cnoe.localtest.me:9443/giteaAdmin/idpbuilder-dummy-my-app-manifests   https://argocd.cnoe.localtest.me:9443/applications/argocd/my-app       true

app2-guestbook2       idpbuilder-dummy   https://github.com/cnoe-io/stacks/tree/main/basic/package2                          https://argocd.cnoe.localtest.me:9443/applications/argocd/guestbook2   true

You mean ArgoCD application URL or git repositories used by ArgoCD applications?

@cmoulliard
Copy link
Contributor Author

Let's merge all files under the types directory. I don't think we should have a file for each struct. It's very Java like right now.😅

Indeed but that simplify the readability of the code and Java language rocks from this point of view :-)

@cmoulliard
Copy link
Contributor Author

You mean ArgoCD application URL or git repositories used by ArgoCD applications?

Argocd URL on our IDP to access the application straight from the Argocd UI

@nabuskey
Copy link
Collaborator

nabuskey commented Jan 3, 2025

Let's merge all files under the types directory. I don't think we should have a file for each struct. It's very Java like right now.😅

Indeed but that simplify the readability of the code and Java language rocks from this point of view :-)

Yeah I'd rather keep it uniform within this project. If you look at how we define related types for this project, we do that in a single file like here: https://github.com/cnoe-io/idpbuilder/blob/main/api/v1alpha1/custom_package_types.go

So let's merge them into a single file.

@nabuskey
Copy link
Collaborator

nabuskey commented Jan 3, 2025

You mean ArgoCD application URL or git repositories used by ArgoCD applications?

Argocd URL on our IDP to access the application straight from the Argocd UI

I see, then we should change the name of the column. It's currently named ARGOCD REPOSITORY. It should be ARGOCD URL yea?

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