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.

Sign up for GitHub

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

Jump to bottom

Enable SCC access logging if scc_access_logging variable is set #1519

Merged
merged 13 commits into from
Mar 4, 2024
Merged

Enable SCC access logging if scc_access_logging variable is set #1519

merged 13 commits into from
Mar 4, 2024

Conversation

mtravitzky
Copy link
Contributor

@mtravitzky mtravitzky commented Feb 28, 2024 •
edited by nodeg
Loading

This will enable the SCC access logger in testing deployments on the server if the variable scc_access_logging is defined in the terraform configuration file. This additional logging is needed for additional tests against SCC flooding in the test suite.

@mtravitzky
Enable SCC access logging by default
9ce6809 
This will enable the SCC access logger in testing deployments
@mtravitzky mtravitzky self-assigned this Feb 28, 2024
Bischoff
Bischoff reviewed Feb 28, 2024
View reviewed changes
Copy link
Contributor

@Bischoff Bischoff left a comment

Choose a reason for hiding this comment

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

See nitpicks

modules/server/variables.tf Outdated Show resolved Hide resolved
modules/server_containerized/variables.tf Outdated Show resolved Hide resolved
srbarrios
srbarrios requested changes Feb 28, 2024
View reviewed changes
Copy link
Member

@srbarrios srbarrios left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please, review my comments.
The most important request is to don't enable this by default.
But there are other request that I would guess are critical for the correct behavior, like how we modify values in the containerized server.

ktsamis
ktsamis approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@ktsamis ktsamis left a comment

Choose a reason for hiding this comment

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

Looks good

ktsamis
ktsamis previously requested changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@ktsamis ktsamis left a comment

Choose a reason for hiding this comment

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

Nope sorry we shouldn't enable by default for all servers. Oscar's review is more thorough

srbarrios
srbarrios reviewed Feb 28, 2024
View reviewed changes
salt/server/tomcat.sls Outdated Show resolved Hide resolved
srbarrios
srbarrios reviewed Feb 28, 2024
View reviewed changes
salt/server/tomcat.sls Outdated Show resolved Hide resolved
srbarrios
srbarrios reviewed Feb 28, 2024
View reviewed changes
salt/server/tomcat.sls Outdated Show resolved Hide resolved
@srbarrios srbarrios requested a review from a team February 28, 2024 12:04
@mtravitzky
Remove changes regarding server_containerized
8fdff64 
These changes can't be finalized at the moment.
mtravitzky and others added 3 commits February 28, 2024 14:31
@mtravitzky @srbarrios
Don't enable SCC logging by default
e2f9676 
Co-authored-by: Oscar Barrios <obarrios@suse.com>
@mtravitzky
Fix jinja code
543e2bc 
The code before had some mistakes and bad design.
@mtravitzky
Merge branch 'enable-scc-access-logging' of github.com:mtravitzky/sum…
639313f 
…aform

Merged current changes from suggestions.
srbarrios
srbarrios reviewed Feb 28, 2024
View reviewed changes
salt/server/tomcat.sls Outdated Show resolved Hide resolved
@mtravitzky
Fixed typo in description
aa4c8a6 
Changed to a capital E
srbarrios
srbarrios reviewed Feb 28, 2024
View reviewed changes
salt/server/tomcat.sls Outdated Show resolved Hide resolved
srbarrios
srbarrios reviewed Feb 28, 2024
View reviewed changes
salt/server/tomcat.sls Outdated Show resolved Hide resolved
srbarrios
srbarrios reviewed Feb 28, 2024
View reviewed changes
salt/server/tomcat.sls Outdated Show resolved Hide resolved
@mtravitzky
Apply some suggestions
5e57a9d 
Applied some improvements to taskomatic.sls and tomcat.sls
srbarrios
srbarrios approved these changes Feb 28, 2024
View reviewed changes
Copy link
Member

@srbarrios srbarrios left a comment

Choose a reason for hiding this comment

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

Thank you so much. LGTM.

NamelessOne91
NamelessOne91 approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@NamelessOne91 NamelessOne91 left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks Marc !

nodeg
nodeg approved these changes Feb 29, 2024
View reviewed changes
Copy link
Member

@nodeg nodeg left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you, Marc!

@nodeg nodeg dismissed ktsamis’s stale review February 29, 2024 12:15

Concerns already addressed and reviewed again by other team members.

Bischoff
Bischoff approved these changes Mar 1, 2024
View reviewed changes
Copy link
Contributor

@Bischoff Bischoff left a comment

Choose a reason for hiding this comment

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

LGTM

@nodeg nodeg changed the title Enable SCC access logging by default Enable SCC access logging if scc_access_logging variable is set Mar 1, 2024
srbarrios
srbarrios reviewed Mar 4, 2024
View reviewed changes
salt/server/tomcat.sls Outdated Show resolved Hide resolved
@mtravitzky @srbarrios
Fix Jinja syntax
33afe2d 
Added spaces

Co-authored-by: Oscar Barrios <obarrios@suse.com>
@mtravitzky mtravitzky merged commit f4e765e into uyuni-project:master Mar 4, 2024
2 checks passed
@mtravitzky mtravitzky mentioned this pull request Mar 8, 2024
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nodeg nodeg nodeg approved these changes

@NamelessOne91 NamelessOne91 NamelessOne91 approved these changes

@srbarrios srbarrios srbarrios approved these changes

@Bischoff Bischoff Bischoff approved these changes

@ktsamis ktsamis Awaiting requested review from ktsamis

@agraul agraul Awaiting requested review from agraul

Assignees

@mtravitzky mtravitzky

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mtravitzky @Bischoff @srbarrios @nodeg @ktsamis @agraul @NamelessOne91