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

OTA-2487: OTA-2544: Remove the secondary discovery mechanism #1191

Merged
merged 3 commits into from
Apr 29, 2019

Conversation

mike-sul
Copy link
Collaborator

As it was decided to preconfigure Primary with Secondary IPs the current discovery mechanism becomes irrelevant.

@codecov-io
Copy link

codecov-io commented Apr 24, 2019

Codecov Report

Merging #1191 into master will increase coverage by 7.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1191      +/-   ##
==========================================
+ Coverage   77.22%   84.36%   +7.13%     
==========================================
  Files         173      228      +55     
  Lines       10112    17036    +6924     
==========================================
+ Hits         7809    14372    +6563     
- Misses       2303     2664     +361
Impacted Files Coverage Δ
src/libaktualizr/config/config.cc 94.7% <ø> (-0.56%) ⬇️
.../aktualizr_secondary/aktualizr_secondary_config.cc 94.36% <ø> (+5.47%) ⬆️
...c/aktualizr_secondary/aktualizr_secondary_config.h 100% <ø> (ø) ⬆️
src/libaktualizr/config/config.h 100% <ø> (ø) ⬆️
src/libaktualizr/config/config_test.cc 98.85% <ø> (ø)
src/libaktualizr-posix/asn1/asn1_test.cc 95.13% <100%> (ø)
src/aktualizr_secondary/main.cc 80% <100%> (+9.41%) ⬆️
src/libaktualizr-posix/asn1/asn1_message.h 79.16% <100%> (+2.24%) ⬆️
src/libaktualizr-posix/asn1/asn1-cerstream.cc 35.26% <0%> (-14.46%) ⬇️
src/libaktualizr-posix/asn1/asn1-cer.cc 89.9% <0%> (-4.59%) ⬇️
... and 62 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de05467...3f3d442. Read the comment docs.

@pattivacek
Copy link
Collaborator

When I run grep discovery -rIin * --exclude-dir=build/ --exclude-dir=partial/ --exclude=tags --exclude=cscope.* I still see some things. Some may still be relevant, but the actions, config, and docs can almost certainly get cleansed of these references.

@mike-sul
Copy link
Collaborator Author

When I run grep discovery -rIin * --exclude-dir=build/ --exclude-dir=partial/ --exclude=tags --exclude=cscope.* I still see some things. Some may still be relevant, but the actions, config, and docs can almost certainly get cleansed of these references.

Yeah, your're right, i just removed the check-discovery and the Secondary's part of discovery, while the Primary's part and corresponding tests still there. Will update this PR.

@mike-sul mike-sul force-pushed the refact/OTA-2544/remove-secondary-discovery branch 2 times, most recently from 2bcb219 to 544a07f Compare April 26, 2019 09:29
@OYTIS
Copy link
Contributor

OYTIS commented Apr 26, 2019

This pull request fixes 1 alert when merging 544a07f into 2a32803 - view on LGTM.com

fixed alerts:

  • 1 for FIXME comment

Comment posted by LGTM.com

Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

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

Looks about right, but I still see a couple discovery references in src/libaktualizr-posix/asn1/messages/ipuptane_message.asn1 that I think can be removed.

@pattivacek
Copy link
Collaborator

Also looks like there is a really pedantic clang-format complaint.

@mike-sul mike-sul force-pushed the refact/OTA-2544/remove-secondary-discovery branch from 544a07f to d59193d Compare April 26, 2019 13:49
Mike Sul added 2 commits April 26, 2019 16:50
Signed-off-by: Mike Sul <ext-mykhaylo.sul@here.com>
Signed-off-by: Mike Sul <ext-mykhaylo.sul@here.com>
@mike-sul mike-sul force-pushed the refact/OTA-2544/remove-secondary-discovery branch from d59193d to fa5c80a Compare April 26, 2019 14:39
@mike-sul
Copy link
Collaborator Author

Looks about right, but I still see a couple discovery references in src/libaktualizr-posix/asn1/messages/ipuptane_message.asn1 that I think can be removed.
Also looks like there is a really pedantic clang-format complaint.

Should be fine now.

@OYTIS
Copy link
Contributor

OYTIS commented Apr 26, 2019

This pull request fixes 1 alert when merging fa5c80a into de05467 - view on LGTM.com

fixed alerts:

  • 1 for FIXME comment

Comment posted by LGTM.com

Signed-off-by: Mike Sul <ext-mykhaylo.sul@here.com>
@mike-sul mike-sul force-pushed the refact/OTA-2544/remove-secondary-discovery branch from fa5c80a to 3f3d442 Compare April 26, 2019 16:43
@mike-sul
Copy link
Collaborator Author

Also looks like there is a really pedantic clang-format complaint.

maybe, it's too pedantic :)

@OYTIS
Copy link
Contributor

OYTIS commented Apr 26, 2019

This pull request fixes 1 alert when merging 3f3d442 into de05467 - view on LGTM.com

fixed alerts:

  • 1 for FIXME comment

Comment posted by LGTM.com

@pattivacek pattivacek merged commit 391db11 into master Apr 29, 2019
@pattivacek pattivacek deleted the refact/OTA-2544/remove-secondary-discovery branch April 29, 2019 06:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants