Skip to content

Comments

[service][web] Allow downloading logs from web UI#379

Merged
dgdavid merged 32 commits intomasterfrom
provide_logs
Jan 18, 2023
Merged

[service][web] Allow downloading logs from web UI#379
dgdavid merged 32 commits intomasterfrom
provide_logs

Conversation

@jreidinger
Copy link
Contributor

@jreidinger jreidinger commented Dec 20, 2022

Problem

There is missing easy way to get logs from d-installer.

Solution

Provide a D-Bus method that returns the path to logs. As a first step, it will just provide YaST logs, but it can be extended in the future with whatever is needed.

Testing

  • Tested manually
  • Added missing unit tests

PR's side effects

For placing the button in a sensible, not intrusive location, we have had to introduce the basic infrastructure for a sidebar controlled by the hamburger menu icon and move few things there. I.e., we have set the foundations for #395. So reviewers: please do not pay so much attention to the current look & feel and add your comments in the linked issue.

Another side effect is that we had to remove the use of the Layout component in the renders test utils functions. Now, the below line has to be used when testing a component making use of the Layout slots (a.k.a. React Portals). Believe or not, it makes testing less painful.

jest.mock("@components/layout/Layout", () => mockLayout());

^^^ Somehow related with #392 and #392 (comment)

Future Steps

  • CLI support
  • Web log viewer

Screenshots

Sidebar hidden Sidebar visible
Screen Shot 2023-01-17 at 17 22 36 Screen Shot 2023-01-17 at 17 22 59
Downloading logs Failed download
Screen Shot 2023-01-17 at 17 23 22 Screen Shot 2023-01-18 at 07 55 17

@coveralls
Copy link

coveralls commented Dec 20, 2022

Coverage Status

Coverage: 75.821% (+0.2%) from 75.576% when pulling f83ebeb on provide_logs into 5d045aa on master.

@dgdavid dgdavid force-pushed the provide_logs branch 2 times, most recently from 917adfc to 2151bbb Compare January 13, 2023 10:43
@dgdavid

This comment was marked as outdated.

@dgdavid

This comment was marked as outdated.

Now it's a one-off click link instead of forcing the user to click twice in
order to collect & download the logs. It shows information about what is going
on too.
@dgdavid dgdavid changed the title Provide logs [service][web] Allow downloading logs from web UI Jan 17, 2023
Don't worry about those sizes by now, most probably they will be changed a few
times more before reaching a certain level of stability.
@dgdavid dgdavid marked this pull request as ready for review January 17, 2023 17:28
@dgdavid dgdavid mentioned this pull request Jan 18, 2023
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

In general, it looks good, although I do not feel comfortable with the naming at all.

const link = await screen.findByLabelText(/Open/i);
const clickEvent = createEvent.click(link);
fireEvent(link, clickEvent);
expect(clickEvent.defaultPrevented).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

The real problem is that opening the sidebar takes you to a different URL. If there is no other way to test that, I am fine with it. Otherwise, we should not extend this kind of tests to other components (just saying).

Copy link
Contributor

@dgdavid dgdavid Jan 18, 2023

Choose a reason for hiding this comment

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

Sure, this is a very specific test that should not be used as an example for other components. But I want to keep it because the intention is to avoid a regression when opening the sidebar: I myself faced the problem of accidentally navigating from the Product Selection page to the Overview when opening the sidebar.

Hopefully, this will change in the near future, after fixing the missuses of button and hiperlink.

In no particular order, below are the next UI changes I'd like to make

^^^ An excerpt from #391 description. ^^^


I see (and share) your concern about the use of fireEvent, but I think we're making a legitimate use of it here. From the @testing-library/dom-testing-library API documentation:

Note

Most projects have a few use cases for fireEvent, but the majority of the time you should probably use @testing-library/user-event.

Also, from the guide-events documentation

Based on the Guiding Principles, your test should resemble how users interact with your code (component, page, etc.) as much as possible. With this in mind, you should know that fireEvent isn't exactly how the user interacts with your application, but it's close enough for most scenarios.

That's why, as you said, we should not extend this kind of tests to other components.

dgdavid and others added 3 commits January 18, 2023 12:12
Co-Authored-By: Imobach González Sosa <igonzalezsosa@suse.com>
Also add missing unit tests to that part.

Co-Authored-By: Imobach González Sosa <igonzalezsosa@suse.com>
@dgdavid dgdavid requested a review from imobachgs January 18, 2023 17:10
@dgdavid dgdavid merged commit 874b266 into master Jan 18, 2023
@dgdavid dgdavid deleted the provide_logs branch January 18, 2023 21:58
lslezak added a commit that referenced this pull request Jan 24, 2023
@imobachgs imobachgs mentioned this pull request Feb 13, 2023
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Feb 16, 2023
https://build.opensuse.org/request/show/1066104
by user IGonzalezSosa + dimstar_suse
- Version 0.7
- Update the list of patterns to install for Leap Micro 5.3
  (gh#agama-project/agama#427).

- Better handling of software repositories
  (gh#agama-project/agama#414):
  * Report issues when reading the software repositories.
  * Inform the user about the software proposal progress.
  * Do not try to calculate a proposal if there are no
    repositories.

- Use the upstream version of D-Bus ObjectManager
  (gh#agama-project/agama#245)

- Save logs and provide the path to the file
  (gh#agama-project/agama#379)

- Implement validation of software proposal
  (gh#agama-project/agama#381)

- Check for installed packages in the target system, instead of the
  installation medium (gh#agama-project/agama#393).

- Simplify the network configuration to just copying the
  NetworkManager connections and e
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Feb 16, 2023
https://build.opensuse.org/request/show/1066106
by user IGonzalezSosa + dimstar_suse
- Version 0.7
- Do not use a proxy to get the errors lists
  (gh#agama-project/agama#424).

- Add live reloading feature for easing the front-end development
  process (gh#agama-project/agama#419).

- Fix storage section crashing when proposal is not ready
  (gh#agama-project/agama#418).

- Better handling of software repositories
  (gh#agama-project/agama#414):
  * Report issues when reading the software repositories.
  * Inform the user about the software proposal progress.
  * Add a button to reload the repositories
    (gh#agama-project/agama#388).

- Added a button for displaying the YaST logs
  (related to gh#agama-project/agama#379)

- UI fixes (gh#agama-project/agama#401):
  * Add a fallback height for the layout
  * Fix some miss-alignments
  * Add missing icon
  * Ensure tooling serving and loading fo
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.

5 participants