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

namespace list view with new api #657

Draft
wants to merge 30 commits into
base: main
Choose a base branch
from
Draft

namespace list view with new api #657

wants to merge 30 commits into from

Conversation

ShaiahWren
Copy link
Contributor

@ShaiahWren ShaiahWren commented Jun 26, 2023

[DO NOT MERGE- waiting on David's work on the ns api to be finished.]

This pr uses the new namespace api for the list and detail views. Additionally, it fixes the namespace create button for use with the correctly typed HubNamespace.

NOTES:

  • I am using the Collection Version Search api with the new useCollectionVersionColumns hook.
  • The namespace details tab is complete, as is its edit form, but patch and delete requests are not allowed yet on the api side, so saving forms will error.

Please use:
ansible/galaxy_ng#1705
pulp/pulp_ansible#1437

@ShaiahWren ShaiahWren requested a review from himdel June 26, 2023 15:12
@github-actions github-actions bot added the HUB label Jun 26, 2023
@himdel
Copy link
Contributor

himdel commented Jun 26, 2023

Might want to make this a draft before the API changes are merged and released?

@himdel himdel mentioned this pull request Jun 26, 2023
@ShaiahWren ShaiahWren requested a review from himdel June 26, 2023 19:15
@ShaiahWren ShaiahWren marked this pull request as draft June 26, 2023 19:38
@himdel
Copy link
Contributor

himdel commented Jun 27, 2023

I'm pretty sure we'll have to fix any type errors before merging instead of hiding them.
Otherwise, looks good 👍

EDIT: we'll try to make the typescript rules a bit more relaxed, will discuss on the next AAP meeting (7/11)

@ShaiahWren ShaiahWren force-pushed the shaiahwren-ns-list branch from fbe19b0 to 3f385ac Compare June 29, 2023 14:45
@ShaiahWren
Copy link
Contributor Author

@himdel a little note on the typing errors... on lines 37 in NamespaceDetails.tsx the typing can't find 'results' or 'count' in the HubNamespace type, so it complains. Is this a case where typing can be loosened or do we have a different option for making tsc happier?

@himdel
Copy link
Contributor

himdel commented Jul 3, 2023

it should not be a problem in HubNamespace but in HubItemsResponse .. the API response you're getting back is not just the namespace, it's something like { data: the_namespace, meta: { count } } (maybe it's not data but results[0] but the point is the response is an object containing the namespace, not the namespace directly).

So .. likely the typing for the namespace itself is just fine, but HubItemsResponse assumes it will be in items and not data or results.

const toolbarFilters = useCollectionFilters();
const tableColumns = useCollectionColumns();
const view = useHubView<Collection>({
url: hubAPI`/_ui/v1/repo/published/`,
Copy link
Contributor

@MilanPospisil MilanPospisil Jul 10, 2023

Choose a reason for hiding this comment

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

This looks like it returns only collections from published repo. And it uses old API, it should use new collection_versions API, like the old application. For example old hub calls in namespace detail:

http://localhost:8002/api/automation-hub/v3/plugin/ansible/search/collection-versions/?namespace=autohubtest2&repository_label=!hide_from_search&is_highest=true&offset=0&limit=10

I have this in my PR: #683
Approvals uses the collection_version API just like collection list.

Note that you need new key function, not the keyFn: idKeyFn, but something else, for example:
keyFn: (item) => item.collection_version.pulp_href + ':' + item.repository.name,

Its because idKeyFN returns id, but the new collection API not returns id, it return collection_version object and repository object. And collection pulp_href cannot be used by itself, because the same collection can be in multiple repos. Thus we add repo name to the key and make it unique.

Feel free to reach me when you have some questions!

@ShaiahWren ShaiahWren force-pushed the shaiahwren-ns-list branch from 3f385ac to 13b6d1e Compare July 10, 2023 19:55
@ShaiahWren
Copy link
Contributor Author

@himdel yeah, you're right, thanks for looking at the typing:) I went ahead and created a new HubNamespaceResponse that works a lot better; I was able to push without applying the tsc ignore:) Lmk if this looks right

@ShaiahWren ShaiahWren requested a review from himdel July 10, 2023 20:19
@ShaiahWren ShaiahWren force-pushed the shaiahwren-ns-list branch 2 times, most recently from 7444329 to 0a249fd Compare July 17, 2023 16:17
Copy link
Contributor

@nixocio nixocio left a comment

Choose a reason for hiding this comment

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

Good job @ShaiahWren, minor suggestions.

import { RouteObj } from '../../../Routes';
import { CollectionVersionSearch } from '../CollectionVersionSearch';

export function useCollectionVersionColumns(_options?: {
Copy link
Contributor

@nixocio nixocio Jul 17, 2023

Choose a reason for hiding this comment

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

We are not passing down the _options prop, neither using the destructed variables. Remove?

Suggested change
export function useCollectionVersionColumns(_options?: {
export function useCollectionVersionColumns(

groups: string[];
}

export interface HubNamespace {
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
export interface HubNamespace {
export interface HubNamespace {
description?: string;
company?: string;


export function NamespaceDetails() {
const { t } = useTranslation();
const params = useParams<{ id: string }>();
const { data } = useGet<HubItemsResponse<HubNamespace>>(
hubAPI`/_ui/v1/namespaces/?limit=1&name=${params.id ?? ''}`
const { data } = useGet<HubNamespaceResponse<HubNamespace>>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Using ItemsResponse seems to fix the typescript issues. I just looked at useGet usage in other contexts. Not sure if It is not addressing something in particular here.

@ShaiahWren ShaiahWren marked this pull request as ready for review July 17, 2023 19:19
@ShaiahWren ShaiahWren force-pushed the shaiahwren-ns-list branch from 746903c to 0872d53 Compare July 17, 2023 19:24
Comment on lines +25 to +36
export interface HubNamespaceResponse<T extends object> {
count: number;
results: T[];
}

Copy link
Contributor

Choose a reason for hiding this comment

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

you may want to replace this one with PulpItemsResponse from usePulpView - that matches the {count, results[], next?} format

himdel
himdel previously approved these changes Jul 18, 2023
Copy link
Contributor

@himdel himdel left a comment

Choose a reason for hiding this comment

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

see note above, but approving either way :)

nixocio
nixocio previously approved these changes Jul 18, 2023
@ShaiahWren
Copy link
Contributor Author

@himdel @nixocio Thank you for your reviews and approvals:)
I can't merge this until David merges the /v3/namespaces work, but once he does, this will be good to go.:)

@ShaiahWren ShaiahWren marked this pull request as draft July 24, 2023 15:00
@ShaiahWren ShaiahWren force-pushed the shaiahwren-ns-list branch from 0872d53 to 26214ac Compare July 25, 2023 19:37
unselectItemsAndRefresh: (items: T[]) => void;
};

export function usePulpSearchView<T extends object>({
Copy link
Contributor

Choose a reason for hiding this comment

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

(this file will not be needed, use useHubView)

export function hubAPI(strings: TemplateStringsArray, ...values: string[]) {
let base = process.env.HUB_API_BASE_PATH;
if (!base) {
if (activeAutomationServer?.type === AutomationServerType.Galaxy) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(remember to rebase, this file already exists on main, and the activeAutomationServer logic is obsolete)

@ShaiahWren ShaiahWren dismissed stale reviews from nixocio and himdel via a0af763 July 10, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants