Skip to content

Modify hook on verified status check#3974

Merged
joshri merged 5 commits intomainfrom
get-object-verified-source
Aug 31, 2023
Merged

Modify hook on verified status check#3974
joshri merged 5 commits intomainfrom
get-object-verified-source

Conversation

@joshri
Copy link
Copy Markdown
Contributor

@joshri joshri commented Aug 30, 2023

I was asked about what the verified column meant on the automations table, and I noticed a potential improvement where we wouldn't have to filter through all the sources every time we display the status, and instead just request the object we need directly (and only running the request if it's a Git repo or OCI repo in the first place). Is this a thing?

@joshri joshri added the area/ui Issues that require front-end work label Aug 30, 2023
@joshri joshri requested a review from AlinaGoaga August 30, 2023 15:21
@joshri joshri force-pushed the get-object-verified-source branch from b846798 to 9833969 Compare August 30, 2023 21:35
Comment thread ui/components/VerifiedStatus.tsx Outdated
export const VerifiedStatus = ({
const checkVerifiable = (sourceRef: ObjectRef): boolean => {
const { name, namespace, kind } = sourceRef;
//can sourceRef actually return undefined stuff?! Typescript says it can.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yup just double checking in objects.ts and it looks like it's defined as sourceRef(): ObjectRef | undefined

kind = "",
clusterName = "",
} = sourceRef || {};
const { data: verifiable } = useGetObject<VerifiableSource>(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe I'd leave this as { data: source } and just send it as source={source} like you did above also for consistency?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This actually causes a duplicate identifier error but I wish it didn't!

@AlinaGoaga
Copy link
Copy Markdown
Contributor

Hey Josh, this looks good to me! It's code moved from Backstage, should probably look into something similar there but we'll leave that for later 🤔 I think the VerifiableResource interface was initially built like that to be more extensible though you are correct atm we're looking at Git Repositories | OCI repositories only as having the potential to be verified so we can leave it in the new format.

@joshri
Copy link
Copy Markdown
Contributor Author

joshri commented Aug 31, 2023

Hey Josh, this looks good to me! It's code moved from Backstage, should probably look into something similar there but we'll leave that for later 🤔 I think the VerifiableResource interface was initially built like that to be more extensible though you are correct atm we're looking at Git Repositories | OCI repositories only as having the potential to be verified so we can leave it in the new format.

Ah I see - I switched it up so that I could just pass any source down to the component and then verify it later - if we change it to FluxObject that's about as extensible as it gets but yes we can address later if it comes up. Thanks for the quick review!

@joshri joshri marked this pull request as ready for review August 31, 2023 20:02
@joshri joshri merged commit 8564a2f into main Aug 31, 2023
@joshri joshri deleted the get-object-verified-source branch August 31, 2023 20:18
waleedhammam pushed a commit that referenced this pull request Sep 3, 2023
* modify hook on verified status check

* extra variable

* remove export into SourcesTable

* refactoring to one component

* edit comment explaining verification check
AsmaaNabilBakr pushed a commit that referenced this pull request Sep 5, 2023
* modify hook on verified status check

* extra variable

* remove export into SourcesTable

* refactoring to one component

* edit comment explaining verification check
TheGostKasper added a commit that referenced this pull request Oct 2, 2023
* Modify hook on verified status check (#3974)

* modify hook on verified status check

* extra variable

* remove export into SourcesTable

* refactoring to one component

* edit comment explaining verification check

* Move stray Progressive Delivery text to related page (#3923)

* Move stray PD text to related page

* Update progressive-delivery-flagger-install.mdx

Update link

* Update website/docs/progressive-delivery/progressive-delivery-flagger-install.mdx

Co-authored-by: Yiannis <8741709+yiannistri@users.noreply.github.com>

* Updates Delivery dashboard

* Replaces !'s with dashes

* Adds necessary yaml for UI login

* Adds line in intro page about browser support

* Adds line breaks attempt number two

* Fixing the line breaks round three

* Fixing icons

* Update website/docs/progressive-delivery/progressive-delivery-flagger-install.mdx

Co-authored-by: Yiannis <8741709+yiannistri@users.noreply.github.com>

---------

Co-authored-by: Yiannis <8741709+yiannistri@users.noreply.github.com>

* update tabs view

* update hight

* fix font weight

* remove unused imports

* update tab border

---------

Co-authored-by: Joshua Israel <65822698+joshri@users.noreply.github.com>
Co-authored-by: Lauri Apple <lauri@weave.works>
Co-authored-by: Yiannis <8741709+yiannistri@users.noreply.github.com>
Co-authored-by: Casper <ahmed.shabaan@weave.works>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ui Issues that require front-end work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants