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

Fix matching logic for logs from namespace when lines = 0 #5660

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

bawjensen
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #5658
License MIT
Doc PR N/A (?)

@CLAassistant
Copy link

CLAassistant commented Aug 19, 2023

CLA assistant check
All committers have signed the CLA.

Comment on lines +101 to +105
var isMatchingProcess = id === 'all'
|| packet.process.name === id
|| packet.process.pm_id === id
|| packet.process.namespace === id;
if (!isMatchingProcess)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

separating out isMatchingProcess felt more readable to me - happy to revert back to something like this instead:

        if (id !== 'all'
          && packet.process.name != id
          && packet.process.pm_id != id
          && packet.process.namespace != id)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the most recently added test/e2e/logs/ case as a template, hoping to use the most modern test patterns

$pm2 start echo.js --namespace e2e-test-log-namespace

LOG_FILE_BASELINE="${LOG_PATH_PREFIX}/baseline-out.log"
$pm2 logs e2e-test-log-namespace > $LOG_FILE_BASELINE & # backgrounded - will be stopped by `$pm2 delete all`
Copy link
Contributor Author

@bawjensen bawjensen Aug 19, 2023

Choose a reason for hiding this comment

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

I haven't seen any e2e (or programmatic tests) for the pm2 logs command, in order to mimic those. So this is a bit of a shot in the dark, with this approach of backgrounding the process.

Copy link

@hinogi hinogi left a comment

Choose a reason for hiding this comment

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

LGTM

@Unitech Unitech merged commit 500ef7d into Unitech:development Oct 6, 2023
1 of 2 checks passed
Unitech added a commit that referenced this pull request Oct 6, 2023
@Unitech
Copy link
Owner

Unitech commented Jan 20, 2024

fixed via pm2@5.3.1
Update:

$ npm install pm2@5.3.1
$ pm2 update

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.

None yet

4 participants