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

changed ports as per issue #3 #4

Merged
merged 8 commits into from
May 17, 2024
Merged

Conversation

hkiiita
Copy link
Contributor

@hkiiita hkiiita commented May 16, 2024

Hi @coderbyheart request you to please review the changes for issue #3 and please comment on any further modifications. Thank you.

@CLAassistant
Copy link

CLAassistant commented May 16, 2024

CLA assistant check
All committers have signed the CLA.

@hkiiita hkiiita mentioned this pull request May 16, 2024
Copy link
Member

@coderbyheart coderbyheart left a comment

Choose a reason for hiding this comment

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

Looks good (trivial change).
However to not break user experience we need to keep the service also exposed on the previously used ports.

@hkiiita
Copy link
Contributor Author

hkiiita commented May 16, 2024

Thanks for replying @coderbyheart ; learnt this new thing today that we indeed need to keep the service alive on old ports for not breaking end user experience. Will work on it and get back to you for more learning and guidance. Thanks again.

@hkiiita
Copy link
Contributor Author

hkiiita commented May 16, 2024

Hi @coderbyheart , please review . Besides the changes made, i did not add old ports in the client.go file as , i guess, we want new clients to access the server on new ports, although, clients with old ports may still continue to access the service.

I have already learnt a thing or two from this work. Looking forward to your further guidance and mentoring on this. Thanks again !

Copy link
Member

@coderbyheart coderbyheart left a comment

Choose a reason for hiding this comment

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

Tests needed that demonstrate that old ports are still used. Client.go can be modified since it's only used for testing.

@hkiiita hkiiita requested a review from coderbyheart May 16, 2024 16:03
@hkiiita
Copy link
Contributor Author

hkiiita commented May 16, 2024

Tests needed that demonstrate that old ports are still used. Client.go can be modified since it's only used for testing.

Hi @coderbyheart , thanks for being with me all along. Have added the tests for both old and new ports, please check and review and let me know for modifications / improvements.

Thanks !

server/server.go Outdated Show resolved Hide resolved
@hkiiita hkiiita requested a review from coderbyheart May 17, 2024 05:28
server/server.go Outdated Show resolved Hide resolved
@coderbyheart
Copy link
Member

Tests pass: https://github.com/NordicPlayground/nordic-developer-academy-coap/actions/runs/9125300745/job/25091193741

client/client.go Outdated
var udpAddr string
var dtlsAddr string
_ = dtlsAddr
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it crept in while testing locally i guess. Removed it. :-)

server/server.go Outdated

}

func launchServer(flagValues flags, udpPort int, dtlsPort int, r *mux.Router) {
Copy link
Member

Choose a reason for hiding this comment

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

Move to new module

Copy link
Contributor Author

@hkiiita hkiiita May 17, 2024

Choose a reason for hiding this comment

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

Thanks for the guidance, yes, moved it like this --> 7938d5c

@hkiiita hkiiita requested a review from coderbyheart May 17, 2024 11:16
@coderbyheart coderbyheart merged commit 1e03518 into NordicPlayground:saga May 17, 2024
1 of 2 checks 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