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

HMS-2590 fix: use 'resources' tag for PATCH /domains/:domain_id #11

Merged

Conversation

avisiedo
Copy link
Contributor

@avisiedo avisiedo commented Sep 28, 2023

Update the generated spec.gen.go file after the new API has been attached.

@avisiedo avisiedo requested a review from tiran September 28, 2023 06:29
@avisiedo avisiedo self-assigned this Sep 28, 2023
@tiran
Copy link
Collaborator

tiran commented Sep 28, 2023

Could you please change the commit message and PR to reflect the details of the change, while focusing on relevant changes? The fact only that spec.gen.go has changed, is not interesting. As a reviewer and developer, I'm interested to know what has in the file.

Also use upper-case letters for title reference

HMS-2590 fix: Regenerate OpenAPI for PATCH /domains/:domain_id

Update to latest OpenAPI spec. `PATCH /domains/:domain_id` now has `resources` tag instead of `actions` tag.

@avisiedo avisiedo changed the title hms-2590 fix: update the openapi generated hms-2590 fix: use 'resources' tag for PATCH /domains/:domain_id Sep 28, 2023
@avisiedo avisiedo changed the title hms-2590 fix: use 'resources' tag for PATCH /domains/:domain_id HMS-2590 fix: use 'resources' tag for PATCH /domains/:domain_id Sep 28, 2023
@avisiedo
Copy link
Contributor Author

@tiran thanks mate! updated!

@tiran
Copy link
Collaborator

tiran commented Sep 28, 2023

You updated the ticket description, but not the commit message that will land in git. The commit message is IMHO more important than the ticket description. After all the information ends up in the commit history.

A sentence or two is more than sufficient for a trivial change. The change doesn't even affect the backend.

@avisiedo avisiedo force-pushed the hms-2590-tags-for-patch-domain branch from 6699a87 to 7406afb Compare September 28, 2023 09:43
@avisiedo
Copy link
Contributor Author

@tiran thanks mate! I didn't realize about the commit message; now it is updated.

@tiran
Copy link
Collaborator

tiran commented Sep 28, 2023

I hate do be pedantic ... but you didn't read my last comment, did you? :) Now the commit message is overly long and goes into unimportant details.

In my opinion and experience, commit messages should provide a short overview of the "what" and "why" of the change set. They should be as long as necessary and as short as possible. For this PR, one sentence, maybe two are more than sufficient. Personally I would not even include an explanation, because the change does not have an impact on the behavior of the backend. It only updates the serialized JSON string of the schema.

@avisiedo avisiedo force-pushed the hms-2590-tags-for-patch-domain branch from 7406afb to 3521ba3 Compare September 29, 2023 07:30
Update the generated spec.gen.go file after the new API has been attached.

Signed-off-by: Alejandro Visiedo <avisiedo@redhat.com>
@avisiedo avisiedo force-pushed the hms-2590-tags-for-patch-domain branch from 3521ba3 to 00cffc8 Compare September 29, 2023 07:33
@avisiedo
Copy link
Contributor Author

avisiedo commented Oct 3, 2023

I hate do be pedantic ... but you didn't read my last comment, did you? :) Now the commit message is overly long and goes into unimportant details.

In my opinion and experience, commit messages should provide a short overview of the "what" and "why" of the change set. They should be as long as necessary and as short as possible. For this PR, one sentence, maybe two are more than sufficient. Personally I would not even include an explanation, because the change does not have an impact on the behavior of the backend. It only updates the serialized JSON string of the schema.

thanks mate! at the beginning I understood it was the PR message; later I updated the commit message; I have updated with a short description. 👍

@frasertweedale frasertweedale self-requested a review October 5, 2023 03:37
@frasertweedale frasertweedale merged commit ffe33a9 into podengo-project:main Oct 5, 2023
1 check passed
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.

3 participants