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

Build and push rootless docker container #8572

Merged
merged 5 commits into from Aug 30, 2023

Conversation

cgdolan
Copy link
Contributor

@cgdolan cgdolan commented Aug 30, 2023

PR checklist:

  • Purpose of the code is evident to future readers
  • Tests included or PR comment includes a reproducible test plan
  • Documentation is up-to-date
  • A changelog entry was added to changelog.d for any user-facing change
  • Change has no security implications (otherwise, ping security team)

If you're unsure about any of this, please see:

@cgdolan cgdolan force-pushed the cgdolan/rootless-docker-build branch from 59a166c to 5ff9867 Compare August 30, 2023 03:23
@cgdolan cgdolan force-pushed the cgdolan/rootless-docker-build branch from b6463ff to dee4d34 Compare August 30, 2023 03:32
@github-actions
Copy link
Contributor

🚫 The whole benchmark suite is too slow: +9.3% (+1.093 s)

14 benchmarks, 9.3% slower on average.

Individual deviations greater than 20% from the baseline are reported. An individual performance degradation of over 30% or a global degradation of over 7% is an error and will block the pull request. See run output for full results ('Show all checks' > 'Tests / semgrep benchmark tests' 'Details').

@github-actions
Copy link
Contributor

🚫 The whole benchmark suite is too slow: +9.0% (+1.090 s)

14 benchmarks, 9.0% slower on average.

Individual deviations greater than 20% from the baseline are reported. An individual performance degradation of over 30% or a global degradation of over 7% is an error and will block the pull request. See run output for full results ('Show all checks' > 'Tests / semgrep benchmark tests' 'Details').

@github-actions
Copy link
Contributor

🚫 The whole benchmark suite is too slow: +7.4% (+1.074 s)

14 benchmarks, 7.4% slower on average.

Individual deviations greater than 20% from the baseline are reported. An individual performance degradation of over 30% or a global degradation of over 7% is an error and will block the pull request. See run output for full results ('Show all checks' > 'Tests / semgrep benchmark tests' 'Details').

Copy link
Collaborator

@aryx aryx left a comment

Choose a reason for hiding this comment

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

Thanks.
Why do we need this though? Was it impossible to make it rootless by default, so we don't need to build 2 images each time? Could you add a comment maybe in the Dockerfile next to the rootless target about why we provide 2 images (I guess it's easier for some customers, but would be nice to have some scenario in which the root one is needed and using the rootless would make too many customers suddently fail or something.

.github/workflows/build-test-docker.yaml Show resolved Hide resolved
.github/workflows/build-test-docker.yaml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
@cgdolan
Copy link
Contributor Author

cgdolan commented Aug 30, 2023

Sure, added a comment in the new nonroot docker stage explaining why we can't just make this the default.

@cgdolan cgdolan merged commit ff099a3 into develop Aug 30, 2023
38 checks passed
@cgdolan cgdolan deleted the cgdolan/rootless-docker-build branch August 30, 2023 18:42
@github-actions
Copy link
Contributor

🚫 The whole benchmark suite is too slow: +7.6% (+1.076 s)

14 benchmarks, 7.6% slower on average.

Individual deviations greater than 20% from the baseline are reported. An individual performance degradation of over 30% or a global degradation of over 7% is an error and will block the pull request. See run output for full results ('Show all checks' > 'Tests / semgrep benchmark tests' 'Details').

@gedigi
Copy link

gedigi commented Sep 2, 2023

Please note just above your change that USER semgrep was disabled because of issues with CircleCI (and GitHub Actions.)

See:

aryx added a commit that referenced this pull request Sep 2, 2023
aryx added a commit that referenced this pull request Sep 2, 2023
This reverts commit ff099a3.


PR checklist:

- [x] Purpose of the code is [evident to future
readers](https://semgrep.dev/docs/contributing/contributing-code/#explaining-code)
- [x] Tests included or PR comment includes a reproducible test plan
- [x] Documentation is up-to-date
- [x] A changelog entry was [added to
changelog.d](https://semgrep.dev/docs/contributing/contributing-code/#adding-a-changelog-entry)
for any user-facing change
- [x] Change has no security implications (otherwise, ping security
team)

If you're unsure about any of this, please see:

- [Contribution
guidelines](https://semgrep.dev/docs/contributing/contributing-code)!
- [One of the more specific guides located
here](https://semgrep.dev/docs/contributing/contributing/)
@aryx
Copy link
Collaborator

aryx commented Sep 2, 2023

reverting this diff @gedigi. Should be fixed in 1.38.3 later today.

cgdolan added a commit that referenced this pull request Sep 5, 2023
cretoxyrhina pushed a commit to cretoxyrhina/semgrep that referenced this pull request Oct 17, 2023
PR checklist:

- [ ] Purpose of the code is [evident to future
readers](https://semgrep.dev/docs/contributing/contributing-code/#explaining-code)
- [ ] Tests included or PR comment includes a reproducible test plan
- [ ] Documentation is up-to-date
- [ ] A changelog entry was [added to
changelog.d](https://semgrep.dev/docs/contributing/contributing-code/#adding-a-changelog-entry)
for any user-facing change
- [ ] Change has no security implications (otherwise, ping security
team)

If you're unsure about any of this, please see:

- [Contribution
guidelines](https://semgrep.dev/docs/contributing/contributing-code)!
- [One of the more specific guides located
here](https://semgrep.dev/docs/contributing/contributing/)
cretoxyrhina pushed a commit to cretoxyrhina/semgrep that referenced this pull request Oct 17, 2023
…grep#8615)

This reverts commit ff099a3.


PR checklist:

- [x] Purpose of the code is [evident to future
readers](https://semgrep.dev/docs/contributing/contributing-code/#explaining-code)
- [x] Tests included or PR comment includes a reproducible test plan
- [x] Documentation is up-to-date
- [x] A changelog entry was [added to
changelog.d](https://semgrep.dev/docs/contributing/contributing-code/#adding-a-changelog-entry)
for any user-facing change
- [x] Change has no security implications (otherwise, ping security
team)

If you're unsure about any of this, please see:

- [Contribution
guidelines](https://semgrep.dev/docs/contributing/contributing-code)!
- [One of the more specific guides located
here](https://semgrep.dev/docs/contributing/contributing/)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants