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

Add query parameters for catalog controller #296

Merged
merged 3 commits into from
Apr 5, 2022
Merged

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Apr 4, 2022

Some small improvements on the catalog controller usage:

  • Error handling in the frontend
  • Provide the option to filter by provider from the backed (api/cluster/checks?provider=azure)
  • Use the flatten version of the data in the frontend
  • Rename get_catalog_by_provider to get_catalog_grouped_by_provider to be more meaningful

@arbulu89 arbulu89 added the enhancement New feature or request label Apr 4, 2022
@arbulu89 arbulu89 force-pushed the query-params-for-catalog branch 2 times, most recently from 9e53ea8 to 12f7147 Compare April 4, 2022 12:54
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Nice! just a tiny thing about a query string parameter.

@@ -76,16 +72,14 @@ const ChecksResults = () => {
const dispatchUpdateCatalog = () => {
dispatch({
type: 'UPDATE_CATALOG',
payload: { flat: '', provider: 'azure' }, //FIXME: Get provider properly
Copy link
Member

Choose a reason for hiding this comment

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

This would translato toa query string like this

/api/checks/catalog?flat=&provider=azure

I would make it flat=true, I find it a bit more robust.

what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current scenario the queries like flat&provider=azure and flat=&provider=azure, both would work. I often time think that booleans are a tricky thing, because each language has their own syntax with true|True|1 etc hehe.

I read that if the query param is a boolean having it without any parameter is pretty normal. But I don't have any issue changing it. What is the most normal thing out there?

Copy link
Member

Choose a reason for hiding this comment

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

in elixir we can check the parameter directly since nil is a falsy value

Copy link
Member

Choose a reason for hiding this comment

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

In the current scenario the queries like flat&provider=azure and flat=&provider=azure, both would work

Yes, they would both work. I just find it a bit inconsistent and ambiguous as a contract.

booleans are a tricky thing, because each language has their own syntax with true|True|1 etc

I see, it is also true that URLs should be independent from the language serving or consuming 😄
If then there is some language constraints in handling urls, that's another topic 😄

That said no big issue for me 👍, anyway we do not yet have real API consumers to take care of.

Copy link
Contributor Author

@arbulu89 arbulu89 Apr 5, 2022

Choose a reason for hiding this comment

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

@fabriziosestito @nelsonkopliku At the end, what is the conclusion? Should I change it?

in elixir we can check the parameter directly since nil is a falsy value

I'm not able to decipher your message @fabriziosestito XD

@arbulu89 arbulu89 force-pushed the query-params-for-catalog branch from 12f7147 to a2471e2 Compare April 4, 2022 13:04
@arbulu89 arbulu89 force-pushed the query-params-for-catalog branch from a2471e2 to 4f0f66e Compare April 4, 2022 13:07
@arbulu89 arbulu89 merged commit f212411 into main Apr 5, 2022
@arbulu89 arbulu89 deleted the query-params-for-catalog branch April 5, 2022 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants