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

refactor(cli): move packages away from internal/engine #2115

Closed
bassosimone opened this issue May 25, 2022 · 0 comments · Fixed by ooni/probe-cli#1034
Closed

refactor(cli): move packages away from internal/engine #2115

bassosimone opened this issue May 25, 2022 · 0 comments · Fixed by ooni/probe-cli#1034
Assignees
@bassosimone
Labels
cleanup There's need to cleanup stuff a bit ooni/probe-engine priority/low refactoring techdebt This issue describes technical debt

Comments

@bassosimone
Copy link
Contributor

bassosimone commented May 25, 2022 •
edited
Loading

This issue is about slowly moving packages from internal/engine into internal and outside of engine. We created internal/engine when we merged https://github.com/ooni/probe-engine into https://github.com/ooni/probe-cli. We started moving packages outside internal/engine following this rule of thumb: review the package before moving, cleanup its implementation, convert to using unit tests for generating coverage. We will close this issue when internal/engine will become empty. (It will take time for this event to occur, but it is actually fine since we are basically moving packages to signal they have been code-reviewed once more and packages below internal/engine are still working as intended, just in need of a cleanup.) Once this effort is done, our code architecture would be cleaner, with flat packages inside internal and the only-historically-significant split between cli and engine would be gone forever.
@bassosimone bassosimone added priority/low refactoring ooni/probe-engine techdebt This issue describes technical debt cleanup There's need to cleanup stuff a bit labels May 25, 2022
@bassosimone bassosimone self-assigned this May 25, 2022
bassosimone added a commit to ooni/probe-cli that referenced this issue May 25, 2022
@bassosimone
cleanup: merge httpheader and httpfailure into model
2bc1a24 
These two small packages could easily be merged into the model
package, since they're clearly model-like packages.

Part of ooni/probe#2115
@bassosimone bassosimone mentioned this issue May 25, 2022
Merged
bassosimone added a commit to ooni/probe-cli that referenced this issue May 25, 2022
@bassosimone
cleanup: merge httpheader and httpfailure into model (#758)
2d721ba 
These two small packages could easily be merged into the model
package, since they're clearly model-like packages.

Part of ooni/probe#2115
bassosimone added a commit to ooni/probe-cli that referenced this issue May 25, 2022
@bassosimone
cleanup: move legacy from internal/engine to internal
aa9a879 
No functional change.

See ooni/probe#2115
@bassosimone bassosimone mentioned this issue May 25, 2022
Merged
bassosimone added a commit to ooni/probe-cli that referenced this issue May 25, 2022
@bassosimone
cleanup: move legacy from internal/engine to internal (#759)
8b0815e 
No functional change.

See ooni/probe#2115
@bassosimone bassosimone mentioned this issue May 30, 2022
Closed
bassosimone added a commit to ooni/probe-cli that referenced this issue May 30, 2022
@bassosimone
refactor(session.go): replace engine/netx with netxlite
ec18cc3 
This diff replaces engine/netx code with netxlite code in
the engine/session.go file. To this end, we needed to move
some code from engine/netx to netxlite. While there, we
did review and improve the unit tests.

A notable change in this diff is (or seems to be) that in
engine/session.go we're not filtering for bogons anymore so
that, in principle, we could believe a resolver returning
to us bogon IP addresses for OONI services. However, I did
not bother with changing bogons filtering because the
sessionresolver package is already filtering for bogons,
so it is actually okay to avoid doing that again the
session.go code. See:

https://github.com/ooni/probe-cli/blob/v3.15.0-alpha.1/internal/engine/internal/sessionresolver/resolvermaker.go#L88

There are two reference issues for this cleanup:

1. ooni/probe#2115

2. ooni/probe#2121
@bassosimone bassosimone mentioned this issue May 30, 2022
Merged
bassosimone added a commit to ooni/probe-cli that referenced this issue May 30, 2022
@bassosimone
refactor(session.go): replace engine/netx with netxlite (#767)
314c3c9 
This diff replaces engine/netx code with netxlite code in
the engine/session.go file. To this end, we needed to move
some code from engine/netx to netxlite. While there, we
did review and improve the unit tests.

A notable change in this diff is (or seems to be) that in
engine/session.go we're not filtering for bogons anymore so
that, in principle, we could believe a resolver returning
to us bogon IP addresses for OONI services. However, I did
not bother with changing bogons filtering because the
sessionresolver package is already filtering for bogons,
so it is actually okay to avoid doing that again the
session.go code. See:

https://github.com/ooni/probe-cli/blob/v3.15.0-alpha.1/internal/engine/internal/sessionresolver/resolvermaker.go#L88

There are two reference issues for this cleanup:

1. ooni/probe#2115

2. ooni/probe#2121
bassosimone added a commit to ooni/probe-cli that referenced this issue Jun 1, 2022
@bassosimone
refactor: move tracex outside of engine/netx
a29c428 
Consistently with ooni/probe#2121 and
ooni/probe#2115, we can now move tracex
outside of engine/netx. The main reason why this makes sense now
is that the package is now changed significantly from the one
that we imported from ooni/probe-engine.

We have improved its implementation, which had not been touched
significantly for quite some time, and converted it to unit
testing. I will document tomorrow some extra work I'd like to
do with this package but likely could not do $soon.
@bassosimone bassosimone mentioned this issue Jun 1, 2022
Merged
bassosimone added a commit to ooni/probe-cli that referenced this issue Jun 1, 2022
@bassosimone
refactor: move tracex outside of engine/netx (#782)
58adb68 
* refactor: move tracex outside of engine/netx

Consistently with ooni/probe#2121 and
ooni/probe#2115, we can now move tracex
outside of engine/netx. The main reason why this makes sense now
is that the package is now changed significantly from the one
that we imported from ooni/probe-engine.

We have improved its implementation, which had not been touched
significantly for quite some time, and converted it to unit
testing. I will document tomorrow some extra work I'd like to
do with this package but likely could not do $soon.

* go fmt

* regen tutorials
@bassosimone bassosimone mentioned this issue Jun 8, 2022
Closed
10 tasks
bassosimone added a commit to ooni/probe-cli that referenced this issue Jan 16, 2023
@bassosimone
feat: rename webconnectivity v0.5 as webconnectivitylte
fb5b9ac 
In this way, we can avoid a package name conflict when moving
the v0.4 webconnectivity inside internal/experiment.

Part of ooni/probe#2115
This was referenced Jan 16, 2023
Merged
Merged
bassosimone added a commit to ooni/probe-cli that referenced this issue Jan 16, 2023
@bassosimone
refactor: prepare for renaming all experiments (#1032)
ecedd2c 
This diff prepares the ground for renaming all experiments. We will move each experiment from internal/engine/experiment/NAME to internal/experiment/NAME. Before we can do that, we need to rename the already existing internal/experiment/webconnectivity to become webconnectivitylte. Otherwise, we will have a naming conflict since there is also internal/engine/experiment/webconnectivity.

See ooni/probe#2115
bassosimone added a commit to ooni/probe-cli that referenced this issue Jan 16, 2023
@bassosimone
refactor: continue moving packages outside of internal/engine (#1031)
c6b275e 
See ooni/probe#2115

Part of ooni/probe#2273
@bassosimone bassosimone mentioned this issue Jan 16, 2023
Merged
4 tasks
bassosimone added a commit to ooni/probe-cli that referenced this issue Jan 16, 2023
@bassosimone
refactor: move all experiments into internal/experiment (#1033)
92b657d 
See ooni/probe#2115 and ooni/probe#2273

While there, fix a test that was broken by c6b275e.
bassosimone added a commit to ooni/probe-cli that referenced this issue Jan 16, 2023
@bassosimone
cleanup: finish moving packages away from engine
c237253 
This diff includes some cleanups while we finish moving packages
away from the internal/engine package.

With this diff, we have finally finished merging probe-engine into
probe-cli, a process initiated in February 2021.

See ooni/probe#1335.

Closes ooni/probe#2115.
@bassosimone bassosimone mentioned this issue Jan 16, 2023
Merged
bassosimone added a commit to ooni/probe-cli that referenced this issue Jan 16, 2023
@bassosimone
cleanup: finish moving packages away from engine (#1034)
815b6fa 
This diff includes some cleanups while we finish moving packages
away from the internal/engine package.

With this diff, we have finally finished merging probe-engine into
probe-cli, a process initiated in February 2021.

See ooni/probe#1335.

Closes ooni/probe#2115.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bassosimone bassosimone

Labels
cleanup There's need to cleanup stuff a bit ooni/probe-engine priority/low refactoring techdebt This issue describes technical debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

cleanup: finish moving packages away from engine ooni/probe-cli
1 participant
@bassosimone