Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

Fix logging of where application is bound #2249

Merged
merged 2 commits into from
May 1, 2022
Merged

Fix logging of where application is bound #2249

merged 2 commits into from
May 1, 2022

Conversation

hut8
Copy link
Contributor

@hut8 hut8 commented Apr 25, 2022

Addresses #2248

This adds a method to a public interface, which isn't ideal and may technically be a breaking change. I'm open to suggestions about that.

@hut8 hut8 marked this pull request as ready for review April 25, 2022 23:44
@hut8 hut8 requested a review from a team as a code owner April 25, 2022 23:44
@hut8
Copy link
Contributor Author

hut8 commented Apr 25, 2022

Log message is now:

 msg="starting server of type *servers.TLS at 0.0.0.0:443"

@sio4 sio4 self-assigned this Apr 26, 2022
sio4
sio4 suggested changes Apr 26, 2022
View reviewed changes
Copy link
Member

@sio4 sio4 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!
I wrote some comments so please kindly consider them.

Briefly, I would like to minimize the changes to the interface since we are now planning a next version and don't want to introduce breaking changes while I fully agreed that the wrong information should be fixed.

server.go Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
servers/servers.go Outdated Show resolved Hide resolved
servers/simple.go Outdated Show resolved Hide resolved
@sio4 sio4 added bug Something isn't working breaking change This feature / fix introduces breaking changes labels Apr 26, 2022
@hut8
Copy link
Contributor Author

hut8 commented Apr 26, 2022 

DEBU[2022-04-26T20:02:55Z] starting application
INFO[2022-04-26T20:02:55Z] starting simple server on 0.0.0.0:3000

That's the new output.

sio4
sio4 approved these changes Apr 27, 2022
View reviewed changes
Copy link
Member

@sio4 sio4 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

@sio4 sio4 removed the breaking change This feature / fix introduces breaking changes label Apr 27, 2022
@sio4
Copy link
Member

sio4 commented Apr 27, 2022

Hi @paganotoni,
Could you please take a look at this PR and merge it? This PR improved the startup message.
(what happen with the codeclimate? 🤔 )

@sio4 sio4 linked an issue Apr 27, 2022 that may be closed by this pull request
Server reports incorrect listening address #2248
Closed
@sio4 sio4 requested review from paganotoni and sio4 April 29, 2022 09:40
@sio4 sio4 changed the base branch from main to development April 29, 2022 14:06
@sio4 sio4 changed the base branch from development to main April 29, 2022 14:07
@sio4 sio4 changed the base branch from main to development April 29, 2022 14:08
@sio4 sio4 changed the base branch from development to main April 29, 2022 14:09
@sio4
Copy link
Member

sio4 commented Apr 29, 2022

Hi @hut8,

I just realized that this PR is opened against the main branch. Currently, all PR requests should open to the development branch. I tried to edit the base but it seems like it could break the commit history. Could you please check your forked repository/branch or reopen the PR against the development branch?

@hut8
Copy link
Contributor Author

hut8 commented Apr 29, 2022 via email

@sio4 sio4 removed the request for review from paganotoni April 30, 2022 01:04
paganotoni
paganotoni approved these changes Apr 30, 2022
View reviewed changes
Copy link
Member

@paganotoni paganotoni left a comment

Choose a reason for hiding this comment

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

LGTM!

@paganotoni paganotoni changed the base branch from main to development April 30, 2022 17:08
@paganotoni
Copy link
Member

@hut8 thanks for putting together this one. I attempted to rebase it to development but seems it has some conflicts. Can you please take care of those. Thanks again!

hut8 added 2 commits May 1, 2022 18:19
@hut8
Fix logging of where application is bound
45e789e
@hut8
Revision as per @sio4
5f675eb 
Makes changes backwards compatible and restores logging statement at
beginning of startup
@hut8
Copy link
Contributor Author

hut8 commented May 1, 2022

@paganotoni I think I took care of it 😄

@paganotoni paganotoni merged commit 7390982 into gobuffalo:development May 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@paganotoni paganotoni paganotoni approved these changes

@sio4 sio4 Awaiting requested review from sio4

Assignees

@sio4 sio4

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Server reports incorrect listening address
3 participants
@hut8 @sio4 @paganotoni