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.

Sign up for GitHub

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

Jump to bottom

Fix 2 string based IP address removal regressions #12506

Merged
merged 2 commits into from
Mar 4, 2020
Merged

Fix 2 string based IP address removal regressions #12506

merged 2 commits into from
Mar 4, 2020

Conversation

kivaisan
Copy link
Contributor

@kivaisan kivaisan commented Feb 25, 2020 •
edited
Loading

Summary of changes

Fix 2 regressions introduced in #11942:

  • Cellular had one trace print using old string based get_ip_address. Fixed to use SocketAddress.
  • L3IPInterface::set_network was not using SocketAddress. Fixed to use SocketAddress also to store ip_address, netmask and gateway.

Fixes: #12486

Impact of changes

Migration actions required

Documentation

Pull request type 

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@ARMmbed/mbed-os-wan @michalpasztamobica

@kivaisan kivaisan changed the title Fix 2 socketaddress regressions Fix 2 string based IP address removal regressions Feb 25, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 25, 2020

This is not yet fixing the report from yesterday:

Docs are now failing one of the code snippet tests: https://travis-ci.org/ARMmbed/mbed-os-5-docs/jobs/653627350#L807

or is it?

@kivaisan
Copy link
Contributor Author

This is not yet fixing the report from yesterday:

Docs are now failing one of the code snippet tests: https://travis-ci.org/ARMmbed/mbed-os-5-docs/jobs/653627350#L807

or is it?

No it's not. In this log the actual error is:
./main.cpp:38:44: error: no matching function for call to 'TLSSocket::connect(const char [12], int)'

0xc0170
0xc0170 previously approved these changes Feb 25, 2020
View reviewed changes
@mergify mergify bot added the needs: CI label Feb 25, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 25, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Feb 25, 2020

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@mergify mergify bot dismissed 0xc0170’s stale review February 25, 2020 12:40

Pull request has been modified.

@kivaisan
Copy link
Contributor Author

@0xc0170 I noticed there was a small bug in L3IPInterface. I pushed a fixed version, so please rerun CI tests.

@ciarmcom ciarmcom requested review from michalpasztamobica and a team February 25, 2020 14:00
@ciarmcom
Copy link
Member

@kivaisan, thank you for your changes.
@michalpasztamobica @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

0xc0170
0xc0170 previously approved these changes Feb 25, 2020
View reviewed changes
@mergify mergify bot added needs: CI and removed needs: review labels Feb 25, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 25, 2020

CI restarted

@mbed-ci
Copy link

mbed-ci commented Feb 25, 2020

Test run: FAILED

Summary: 5 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_mbed2-build-IAR
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-ARM
  • jenkins-ci/mbed-os-ci_cloud-client-pytest
  • jenkins-ci/mbed-os-ci_exporter

@mergify mergify bot removed the needs: CI label Feb 26, 2020
@kivaisan
Copy link
Contributor Author

@0xc0170 Is this still some CI problem as I'm unable to find any actual build failure?

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 27, 2020

Yes, I restarted CI. Client is flaky today, cc @ARMmbed/mbed-os-test

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 2, 2020

Restarted entire pipeline again

@mbed-ci
Copy link

mbed-ci commented Mar 2, 2020

Test run: FAILED

Summary: 1 of 7 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 2, 2020

Tests should be fixed, restarted

@mbed-ci
Copy link

mbed-ci commented Mar 2, 2020

Test run: FAILED

Summary: 1 of 7 test jobs failed
Build number : 5
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-pytest

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 3, 2020

@kivaisan Please rebase, will restart CI asap

@mergify
Copy link

mergify bot commented Mar 3, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

Kimmo Vaisanen added 2 commits March 3, 2020 12:12
Cellular: Fix trace to use new SocketAddress based get_ip_address
6e1f7bf 
String based get_ip_address has been removed and SocketAddress based must be
used.
Fix L3IPInterface to use SocketAddress class for addresses
86dba54 
String based ip addresses has been deprecated and SocketAddress must be
used instead.
@mergify mergify bot dismissed 0xc0170’s stale review March 3, 2020 10:23

Pull request has been modified.

@kivaisan
Copy link
Contributor Author

kivaisan commented Mar 3, 2020

Rebased.

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 3, 2020

CI restarted

@mbed-ci
Copy link

mbed-ci commented Mar 3, 2020

Test run: SUCCESS

Summary: 7 of 7 test jobs passed
Build number : 6
Build artifacts

@0xc0170 0xc0170 merged commit 6677249 into ARMmbed:master Mar 4, 2020
@mergify mergify bot removed the ready for merge label Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@michalpasztamobica michalpasztamobica michalpasztamobica approved these changes

Assignees
No one assigned
Labels
release-version: 6.0.0-alpha-3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Build failure if PPP and mbed trace or L3IPInterface is enabled
5 participants
@kivaisan @0xc0170 @mbed-ci @ciarmcom @michalpasztamobica