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

Feature/1269 add shortcut to tailwarden in inventory table #1306

Conversation

syedbarimanjan
Copy link
Contributor

@syedbarimanjan syedbarimanjan commented Dec 30, 2023

Problem

When a service is not supported, there is no interaction with the user, leading to confusion as the user may perceive the tool as broken. #1269

Solution

I created a servicehelper utility that stores all the providers and their services in an object. Two functions were implemented:

1. checkIfServiceIsSupported(provider: string, service: string): boolean

  • Takes a provider and service as parameters.
  • Returns a boolean indicating whether the service is supported for the given provider.

2. checkIsSomeServiceUnavailable(receivedServices: string[]): boolean

  • Takes an array of services as a parameter.
  • Returns a boolean indicating whether any service in the array is not supported.

In the useInventory hook, I added a new state to check if some service is unavailable. If there is something in the searchedInventory array, I loop over it and use the first function to check if even one element is not a supported service. If true, I set the state. If the searchedInventory is empty, I fetch all services from the API endpoint and use the second function to check if any service is not available. The result is set to the state.

Finally, the state is returned from the useInventory hook, and in the InventoryPage, it is destructured and passed to the InventoryStatsCard. If the state is true, an error icon is displayed, and clicking on it redirects to the Tailwarden page for more information.

Screenshots

image
image

Notes

I have updated the contributing guide to include a reminder for contributors to add any new provider or service to the list in the serviceHelper utility.

Checklist

  • Code follows the contributing guidelines
  • Changes have been thoroughly tested
  • Documentation has been updated, if necessary
  • Any dependencies have been added to the project, if necessary

Reviewers

@Azanul @jakepage91 @mlabouardy @Traxmaxx @ShubhamPalriwala @AvineshTripathi @greghub

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hey, thank you for opening your Pull Request ! 🙂 While a Tailwarden team member takes a look at your PR we would like to invite you to join our official Discord server, where you can interact directly with other contributors and Tailwarden team members. Link here: https://discord.tailwarden.com

dashboard/utils/serviceHelper.ts Outdated Show resolved Hide resolved
dashboard/utils/serviceHelper.ts Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
syedbarimanjan and others added 3 commits January 2, 2024 12:01
Co-authored-by: Azanul Haque <42029519+Azanul@users.noreply.github.com>
@syedbarimanjan
Copy link
Contributor Author

@Azanul did the changes.

@Azanul
Copy link
Collaborator

Azanul commented Jan 2, 2024

Screenshot 2024-01-02 at 12 02 27 PM

@AvineshTripathi
Copy link
Collaborator

image
I find the position to be odd can we move the icon inside the discovered cost part

@@ -0,0 +1,229 @@
// Define a union type for supported cloud providers.
export type Providers =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think having an api that returns the list of resources would be more better. I find this a bit redundant but open to discuss

Copy link
Contributor Author

@syedbarimanjan syedbarimanjan Jan 2, 2024

Choose a reason for hiding this comment

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

there is an api which returns the services/resources which the user has in the database I am utilizing it in the useInventory hook but the problem is how do we identify which one of those services which the user is using are not supported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one thing we could do is that change the /services api to return an object which has the name of the service and then another property which indicates if it is supported or not

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Azanul do you think we can keep the cost field in db as null? if yes will it be a good choice(I feel that would be costly!) the main reason I want to do this is because there is manual work in adding the names in frontend too and I want to keep this as separate. Thoughts?

Copy link
Contributor Author

@syedbarimanjan syedbarimanjan Jan 3, 2024

Choose a reason for hiding this comment

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

Indeed, that approach would work if the service/resource is not supported, the cost should be set to null. This way, we can then handle errors accordingly. But it would be hard to check if the service is supported or not and then setting the cost to null, and that might even have some manual work init.

Copy link
Collaborator

@AvineshTripathi AvineshTripathi Jan 4, 2024

Choose a reason for hiding this comment

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

I think that should be a simple, we can convert the cost field of Resource schema to either be null or float value. This way we can have just call for resoources that have null value in it
ref: https://bun.uptrace.dev/guide/models.html#nulls

Copy link
Contributor Author

@syedbarimanjan syedbarimanjan Jan 4, 2024

Choose a reason for hiding this comment

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

I think that should be a simple, we can convert the cost field of Resource schema to either be null or float value. This way we can have just call for resoources that have null value in it ref: https://bun.uptrace.dev/guide/models.html#nulls

Ok I will try to do this and can you confirm that are we using bun as an orm here and where should I start.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@syedbarimanjan
Copy link
Contributor Author

syedbarimanjan commented Jan 2, 2024

image I find the position to be odd can we move the icon inside the discovered cost part

It is what the designer designed, and I also like it the alternative would be like this
Screenshot 2024-01-02 175510
which doesnt look good

@AvineshTripathi
Copy link
Collaborator

@AllieMendes whats your take on above

@AllieMendes
Copy link

Hey everyone!

The positioning is intentional and ties in with the layout we use at Tailwarden, which creates familiarity for similar features/information.
The fact that it's slightly outside the container also helps highlight it, which is important as the information provides transparency.

It would be great if we could keep the original layout from the designs.

@mlabouardy
Copy link
Collaborator

@Azanul could we merge this one?

@Azanul
Copy link
Collaborator

Azanul commented Jan 16, 2024

@Azanul could we merge this one?

Yes, another approval is needed

@syedbarimanjan
Copy link
Contributor Author

@Azanul could we merge this one?

I am working on implementing this the backend way as suggested by Avinesh but if this is all right then maybe I shouldn't?

@Azanul
Copy link
Collaborator

Azanul commented Jan 16, 2024

@Azanul could we merge this one?

I am working on implementing this the backend way as suggested by Avinesh but if this is all right then maybe I shouldn't?

Although I agree with @AvineshTripathi, this PR looks good. We can work on the recommended changes on a different PR.
We can do the changes here as well, depending completely on your availability.

@syedbarimanjan
Copy link
Contributor Author

@Azanul could we merge this one?

I am working on implementing this the backend way as suggested by Avinesh but if this is all right then maybe I shouldn't?

Although I agree with @AvineshTripathi, this PR looks good. We can work on the recommended changes on a different PR. We can do the changes here as well, depending completely on your availability.

I will INSHAALLAH commit the changes in some days.

@syedbarimanjan
Copy link
Contributor Author

syedbarimanjan commented Jan 25, 2024

@Azanul could we merge this one?

I am working on implementing this the backend way as suggested by Avinesh but if this is all right then maybe I shouldn't?

Although I agree with @AvineshTripathi, this PR looks good. We can work on the recommended changes on a different PR. We can do the changes here as well, depending completely on your availability.

Can you just merge this one when I get time I will raise a new pr for implementing this the backend way. @mlabouardy

@syedbarimanjan
Copy link
Contributor Author

@Azanul could we merge this one?

Yes, another approval is needed

@mlabouardy

@Azanul Azanul merged commit 5e2fb7a into tailwarden:develop Jan 28, 2024
3 checks passed
Azanul added a commit that referenced this pull request Jan 31, 2024
* feat: add service check helpers

* feat(useInventory): track unsupported services state

* feat(ui): enhance ui  to reflect unsupported services

* docs: update contributing guidelines

* docs: update contributing guidelines

* Update CONTRIBUTING.md

* style: suggested changes

Co-authored-by: Azanul Haque <42029519+Azanul@users.noreply.github.com>

* fix: type error

* style: add border radius

---------

Co-authored-by: Azanul Haque <42029519+Azanul@users.noreply.github.com>
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.

5 participants