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.

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

Do not add module to instance settings if error in audit event object #2726

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

knolleary
Copy link
Member

Fixes #2710

Description

If a node install fails, the audit log entry includes an 'error' property. We were not filtering on that, so we believed the install was successful and then adding the node to the instance settings.

This PR fixes that - adds the necessary filtering and adds test coverage for the audit-log handling in this area.

Checklist

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #2726 (39357cf) into main (e2811b1) will increase coverage by 34.94%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main    #2726       +/-   ##
===========================================
+ Coverage   39.52%   74.47%   +34.94%     
===========================================
  Files         540      232      -308     
  Lines       19130     9609     -9521     
  Branches     4523     2019     -2504     
===========================================
- Hits         7562     7156      -406     
+ Misses      11568     2453     -9115     
Flag Coverage Δ
backend 74.47% <100.00%> (+0.02%) ⬆️
frontend ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
forge/routes/logging/index.js 100.00% <100.00%> (+10.00%) ⬆️

... and 394 files with indirect coverage changes

Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

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

👍

@Steve-Mcl
Copy link
Contributor

Odd

  FlowForge - Personal Access Tokens
    ✓ dialog has the correct default state (2180ms)
    ✓ can be added without an expiry (2224ms)
    ✓ can be added with an expiry date (2629ms)
    1) can be removed


  3 passing (13s)
  1 failing

  1) FlowForge - Personal Access Tokens
       can be removed:

      AssertionError: Timed out retrying after 4000ms: Too many elements found. Found '1', expected '-1'.
      + expected - actual

      -1
      +-1
      
      at Context.eval (webpack:///../test/e2e/frontend/cypress/tests/account/tokens.spec.js:98:60)

Wasnt expecting that 🤔

@Steve-Mcl
Copy link
Contributor

I believe @Pezmc had the same UI test failure on an unrelated PR. I think running the test once more it passed.

Seeing that tice suggests a timing issue or something not properly interlocked?

@knolleary
Copy link
Member Author

Yeah, this is an unrelated failure. Going to move ahead with this PR and capture the failure in an issue for looking at separately.

@knolleary knolleary merged commit b82d37a into main Sep 11, 2023
3 of 5 checks passed
@knolleary knolleary deleted the 2710-filter-failed-node-installs branch September 11, 2023 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants