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

Rename route: /software_updates/settings -> /settings/suma_credentials #2264

Merged
merged 6 commits into from
Feb 7, 2024

Conversation

jamie-suse
Copy link
Contributor

Description

Renames the route at /api/v1/software_updates/settings to /api/v1/settings/suma_credentials.

Also updates the routing code to use the resources macro to be more in-line with typical Phoenix routing code as seen in the Phoenix Routing Guide.

How was this tested?

Updated existing unit tests

@jamie-suse jamie-suse added the elixir Pull requests that update Elixir code label Feb 6, 2024
@nelsonkopliku
Copy link
Member

We need to temporarily skip the api bc check in the pipeline

@jamie-suse jamie-suse marked this pull request as ready for review February 6, 2024 14:22
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.

LGTM

Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

LGTM

--json /specs/changes.json \
--text /specs/changes.txt \
--html /specs/changes.html
run: echo "TEMPORARY SKIP"
Copy link
Contributor

Choose a reason for hiding this comment

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

Either we remember to just edit this or I can merge with admin privileges if we all agree on that. I'd rather forcemerge instead of doing 2 PRs but up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'd be awesome if so. I'll revert this change then.

@@ -146,7 +146,7 @@ defmodule TrentoWeb.Router do

get "/hosts/:id/exporters_status", PrometheusController, :exporters_status

get "/software_updates/settings", SoftwareUpdatesController, :get_software_updates_settings
resources "/settings/suma_credentials", SoftwareUpdatesController, only: [:index]
Copy link
Member

Choose a reason for hiding this comment

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

I believe we might want to look at singleton-resources https://hexdocs.pm/phoenix/Phoenix.Router.html#resources/4-singleton-resources

Seems to better fit our usecase.

Suggested change
resources "/settings/suma_credentials", SoftwareUpdatesController, only: [:index]
resources "/settings/suma_credentials", SoftwareUpdatesController, only: [:show], singleton: true

The operation in the controller needs adjusted to :show accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

Let's also rename the controller and file to something like SoftwareUpdatesCredentialsController or even SUMACredentialsController.

It'd be clearer which resource the actions show, update, delete would be referring to

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.

A couple of extra tiny adjustments

@@ -146,7 +146,7 @@ defmodule TrentoWeb.Router do

get "/hosts/:id/exporters_status", PrometheusController, :exporters_status

get "/software_updates/settings", SoftwareUpdatesController, :get_software_updates_settings
resources "/settings/suma_credentials", SoftwareUpdatesController, only: [:index]
Copy link
Member

Choose a reason for hiding this comment

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

Let's also rename the controller and file to something like SoftwareUpdatesCredentialsController or even SUMACredentialsController.

It'd be clearer which resource the actions show, update, delete would be referring to

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.

Thanks! LGTM (again 😄)

@jamie-suse jamie-suse merged commit d0235c9 into main Feb 7, 2024
22 of 23 checks passed
@jamie-suse jamie-suse deleted the rename-suma-route branch February 7, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
elixir Pull requests that update Elixir code
Development

Successfully merging this pull request may close these issues.

None yet

4 participants