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

maint(landscape-mock): Bugfixes and logging in Landcape mock #377

Merged
merged 10 commits into from
Nov 14, 2023
Merged

maint(landscape-mock): Bugfixes and logging in Landcape mock #377

merged 10 commits into from
Nov 14, 2023

Conversation

EduardGomezEscandell
Copy link
Contributor

@EduardGomezEscandell EduardGomezEscandell commented Oct 31, 2023 •
edited
Loading

This PR is a spin-off off #326 . This PR does two things:

  • Introduces logging to the Landscape mock, which is critical to debugging the E2E tests. Controling the verbosity of these logs required more flags in the command line, so I switched to Cobra.
  • Fixes various bugs.

UDENG-1549

@EduardGomezEscandell EduardGomezEscandell self-assigned this Oct 31, 2023
@EduardGomezEscandell EduardGomezEscandell changed the title main(landscape-mock): Bugfixes and logging in Landcape mock maint(landscape-mock): Bugfixes and logging in Landcape mock Oct 31, 2023
@EduardGomezEscandell
Landscape mock: introduce logging
a5f774f
@EduardGomezEscandell
Landscape mock: reorganize main function
0340672 
Now that the mock Landscape service has loging, we need a way of setting
it from the command line. To avoid parsing even more arguments manually,
I switched to Cobra with a very similar structure to that of the mock
contract server.
@EduardGomezEscandell
Landscape mock: fix issue were distros would receive a new UID every …
fb31ab0 
…time
@EduardGomezEscandell
Landscape mock: Fix a panic by refactoring the recv loop
4710da1 
This fixes a panic (write to closed channel) in the tests. The problem
was that multiple goroutines were writing to the same channel, so none
of them could reliably be in charge of closing it. Now there is a single
writing goroutine, which closes the channel on exit.
@EduardGomezEscandell EduardGomezEscandell marked this pull request as ready for review October 31, 2023 11:36
CarlosNihelton
CarlosNihelton reviewed Nov 2, 2023
View reviewed changes
Copy link
Contributor

@CarlosNihelton CarlosNihelton left a comment

Choose a reason for hiding this comment

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

Just noticed a typo.

mocks/landscape/landscapemockservice/landscape_mock_service.go Outdated Show resolved Hide resolved
didrocks
didrocks requested changes Nov 14, 2023
View reviewed changes
Copy link
Collaborator

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

I have quite some cosmetic/minor suggestion, to keep things in line with other projects and avoid the elements of suprprises when going to this mock.

And btw:

so I switched to Cobra.

I would have bet that would happen! :)

mocks/landscape/landscapemockservice/landscape_mock_service.go Outdated Show resolved Hide resolved
mocks/landscape/landscapemockservice/landscape_mock_service.go Outdated Show resolved Hide resolved
mocks/landscape/landscapemockservice/landscape_mock_service.go Outdated Show resolved Hide resolved
mocks/landscape/landscapemockservice/landscape_mock_service.go Outdated Show resolved Hide resolved
mocks/landscape/landscapemockservice/landscape_mock_service.go Show resolved Hide resolved
mocks/landscape/landscaperepl/main.go Outdated Show resolved Hide resolved
mocks/landscape/landscaperepl/main.go Outdated Show resolved Hide resolved
@EduardGomezEscandell
Improve comments
9b167c7
@EduardGomezEscandell
Replace help with "help"
5eac990 
It looks better in terminals
@EduardGomezEscandell
Follow usual convention for Apps
afd2927 
I changed the structure of the code so it more closely resembles the
WSL-Pro-Service and Windows-Agent apps.

The Landscape mock App matches the interface except that it is missing
the Quit method. I did not implement it because I saw no reason to. This
method is used only in signal handling, which we don't use in the
Landscape mock.
@EduardGomezEscandell
Copy link
Contributor Author

so I switched to Cobra.

I would have bet that would happen! :)

Hey, this is only to parse the -vvv and URL commands! The REPL is still hand-crafted 🎨

@EduardGomezEscandell
Improve error check in asyncRecv
aa17113 
We used to check that the message was not nill, but forgot to check the
error.

This double-check (is err nil? + is message nil?) would cause a bit of
ugly nesting so I moved it into newHostInfo so we can return instead of
nest.

This required a renaming of newHostInfo to receiveHostInfo to
keep the naming accurate.
@EduardGomezEscandell EduardGomezEscandell mentioned this pull request Nov 14, 2023
Merged
didrocks
didrocks approved these changes Nov 14, 2023
View reviewed changes
Copy link
Collaborator

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

LGTM now!

@EduardGomezEscandell EduardGomezEscandell merged commit 2bed1ab into main Nov 14, 2023
28 of 30 checks passed
@EduardGomezEscandell EduardGomezEscandell deleted the landscape-mock-fixes branch November 14, 2023 13:45
EduardGomezEscandell added a commit that referenced this pull request Dec 12, 2023
@EduardGomezEscandell
tests(landscape): Add landscape checks in end-to-end tests (#326)
6adcbac 
This pull request sets up the Landscape mock server in both registry and
manually provided token tests.


This PR depends on #377 for Landscape mock service fixes and #398 for
dynamic certificate generation.

---
UDENG-1549
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CarlosNihelton CarlosNihelton CarlosNihelton left review comments

@didrocks didrocks didrocks approved these changes

Assignees

@EduardGomezEscandell EduardGomezEscandell

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@EduardGomezEscandell @didrocks @CarlosNihelton