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

feat(ci): don't log raw data #1067

Merged
merged 1 commit into from Dec 12, 2023

Conversation

RolandSherwin
Copy link
Member

@RolandSherwin RolandSherwin commented Dec 11, 2023

Description

Summary generated by Reviewpad on 11 Dec 23 10:59 UTC

This pull request adds a feature to the CI workflow. It modifies the merge.yml and nightly.yml files to prevent logging of raw data. It includes new steps in the workflow that check the length of the logs and exit with an error message if the length exceeds a certain threshold. The safe_path is also defined for each operating system.

@reviewpad reviewpad bot requested a review from joshuef December 11, 2023 10:59
@reviewpad reviewpad bot added Small Pull request is small waiting-for-review labels Dec 11, 2023
shell: bash
timeout-minutes: 20
run: |
if rg '^' "${{ matrix.safe_path }}"/*/*/logs | awk 'length($0) > 15000 { print; exit 1 }'
Copy link
Contributor

Choose a reason for hiding this comment

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

why 15000 here and not 2k?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I was testing it out. I was going for 15k chars as the limit.
On a local run, I can se log lines upto 4k-5k chars. So 15k seems like something that we might possibly log. Raw data should contain upwards of 100k chars, so this might be a reasonable value to detect outliers but not trigger false positives.

shell: bash
timeout-minutes: 20
run: |
if rg '^' "${{ matrix.safe_path }}"/*/*/logs | awk 'length($0) > 15000 { print; exit 1 }'
Copy link
Contributor

Choose a reason for hiding this comment

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

and here

@RolandSherwin RolandSherwin force-pushed the dont_log_long_lines branch 5 times, most recently from 7f28534 to d612fd1 Compare December 11, 2023 12:19
@RolandSherwin RolandSherwin force-pushed the dont_log_long_lines branch 3 times, most recently from 03fcd9b to f0d7ac1 Compare December 11, 2023 16:50
@joshuef joshuef merged commit 58c044f into maidsafe:main Dec 12, 2023
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Small Pull request is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants