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

Enforce the Dynaconf validaton #15289

Merged
merged 1 commit into from
Jun 18, 2024
Merged

Enforce the Dynaconf validaton #15289

merged 1 commit into from
Jun 18, 2024

Conversation

ogajduse
Copy link
Member

@ogajduse ogajduse commented Jun 4, 2024 •
edited
Loading

Problem Statement

Currently, the configuration is not consistently validated. This can lead to:

  • Uncaught errors in production: Invalid configurations can cause crashes, or unexpected behavior if not detected before deployment.
  • Missed configuration changes: If validators are not updated alongside code changes, invalid configurations might slip through unnoticed.

Solution

Enforce the config validation.

Additionally, remove unused configuration files and their validators.

Benefits:

  • prevent errors - catch configuration issues early to prevent crashes, and unexpected behavior,
  • consistent configs - define valid formats and value ranges to reduce surprises.
  • improve maintainability - validators act as documentation for configuration settings, making it easier for new developers to understand the project.
  • early detection - identify configuration issues during development or deployment to save time and resources compared to fixing them in production.

Notes

I am open to changing the setting name and placement, robottelo.unit_test might not be the best choice.

@ogajduse ogajduse requested a review from a team as a code owner June 4, 2024 08:55
@ogajduse ogajduse added CherryPick PR needs CherryPick to previous branches 6.13.z Introduced in or relating directly to Satellite 6.13 6.14.z Introduced in or relating directly to Satellite 6.14 6.15.z Introduced in or relating directly to Satellite 6.15 6.16.z Dynaconf PRs related to the conversion to Dynaconf and removed Dynaconf PRs related to the conversion to Dynaconf labels Jun 4, 2024
@jyejare jyejare removed the 6.16.z label Jun 5, 2024
JacobCallahan
JacobCallahan requested changes Jun 11, 2024
View reviewed changes
Copy link
Member

@JacobCallahan JacobCallahan left a comment

Choose a reason for hiding this comment

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

Holding on the change requested by @omkarkhatavkar

@ogajduse ogajduse mentioned this pull request Jun 14, 2024
Merged
@Satellite-QE Satellite-QE mentioned this pull request Jun 17, 2024
Merged
@ogajduse
Copy link
Member Author

trigger: test-robottelo

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7405
Build Status: SUCCESS
PRT Comment: pytest /opt/app-root/src/robottelo/tests/foreman -v -m build_sanity --external-logging
Test Result : ======= 12 passed, 5502 deselected, 5608 warnings in 1565.82s (0:26:05) ========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Jun 17, 2024
@JacobCallahan JacobCallahan marked this pull request as draft June 17, 2024 13:44
@ogajduse
Copy link
Member Author

trigger: test-robottelo

@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Jun 17, 2024
dosas
dosas reviewed Jun 17, 2024
View reviewed changes
robottelo/config/__init__.py Outdated Show resolved Hide resolved
@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7407
Build Status: SUCCESS
PRT Comment: pytest /opt/app-root/src/robottelo/tests/foreman -v -m build_sanity --external-logging
Test Result : ======= 12 passed, 5502 deselected, 5612 warnings in 1444.80s (0:24:04) ========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Jun 17, 2024
lpramuk
lpramuk approved these changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@lpramuk lpramuk left a comment

Choose a reason for hiding this comment

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

Wow! So much clean-up on a single place

@ogajduse ogajduse marked this pull request as ready for review June 18, 2024 08:24
@ogajduse ogajduse requested review from a team as code owners June 18, 2024 08:24
@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Jun 18, 2024
jyejare
jyejare reviewed Jun 18, 2024
View reviewed changes
Copy link
Member

@jyejare jyejare left a comment

Choose a reason for hiding this comment

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

ACK pending question.

.github/workflows/pull_request.yml
@@ -9,6 +9,7 @@ env:
PYCURL_SSL_LIBRARY: openssl
ROBOTTELO_BUGZILLA__API_KEY: ${{ secrets.BUGZILLA_KEY }}
ROBOTTELO_JIRA__API_KEY: ${{ secrets.JIRA_KEY }}
ROBOTTELO_ROBOTTELO__SETTINGS__IGNORE_VALIDATION_ERRORS: true
Copy link
Member

Choose a reason for hiding this comment

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

Should we relly ignore the validation errors in checks? Whats the place time to validate them then ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to enable the validation of the production configuration. To enable validation in checks, we would need to spend time fine-tuning the .yaml.template config files here in the conf directory.
This can be a separate effort.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, We have to fine-tune them all to be able to turn it on here.

Copy link
Member

Choose a reason for hiding this comment

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

Created #15443 for tracking purpose.

@jyejare jyejare mentioned this pull request Jun 18, 2024
Open
@jyejare jyejare merged commit 6172869 into SatelliteQE:master Jun 18, 2024
10 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 18, 2024
@ogajduse @web-flow
Enforce the Dynaconf validaton (#15289)
e9ebf70 
(cherry picked from commit 6172869)
@Satellite-QE Satellite-QE mentioned this pull request Jun 18, 2024
Merged
github-actions bot pushed a commit that referenced this pull request Jun 18, 2024
@ogajduse @web-flow
Enforce the Dynaconf validaton (#15289)
e8c5b7a 
(cherry picked from commit 6172869)
github-actions bot pushed a commit that referenced this pull request Jun 18, 2024
@ogajduse @web-flow
Enforce the Dynaconf validaton (#15289)
a4cf56c 
(cherry picked from commit 6172869)
This was referenced Jun 18, 2024
Merged
Merged
@ogajduse ogajduse deleted the feat/force-validation branch June 18, 2024 10:28
jyejare pushed a commit that referenced this pull request Jun 18, 2024
@Satellite-QE @ogajduse
[6.15.z] Enforce the Dynaconf validaton (#15446)
5c385dc 
Enforce the Dynaconf validaton (#15289)

(cherry picked from commit 6172869)

Co-authored-by: Ondřej Gajdušek <[email protected]>
jyejare pushed a commit that referenced this pull request Jun 18, 2024
@Satellite-QE @ogajduse
[6.14.z] Enforce the Dynaconf validaton (#15444)
8aebec9 
Enforce the Dynaconf validaton (#15289)

(cherry picked from commit 6172869)

Co-authored-by: Ondřej Gajdušek <[email protected]>
jyejare pushed a commit that referenced this pull request Jun 18, 2024
@Satellite-QE @ogajduse
[6.13.z] Enforce the Dynaconf validaton (#15445)
4172810 
Enforce the Dynaconf validaton (#15289)

(cherry picked from commit 6172869)

Co-authored-by: Ondřej Gajdušek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omkarkhatavkar omkarkhatavkar omkarkhatavkar left review comments

@dosas dosas dosas approved these changes

@lpramuk lpramuk lpramuk approved these changes

@jyejare jyejare jyejare approved these changes

@jameerpathan111 jameerpathan111 jameerpathan111 approved these changes

@ColeHiggins2 ColeHiggins2 ColeHiggins2 approved these changes

@rmynar rmynar rmynar approved these changes

@JacobCallahan JacobCallahan Awaiting requested review from JacobCallahan

Assignees

@ogajduse ogajduse

Labels
6.13.z Introduced in or relating directly to Satellite 6.13 6.14.z Introduced in or relating directly to Satellite 6.14 6.15.z Introduced in or relating directly to Satellite 6.15 CherryPick PR needs CherryPick to previous branches
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@ogajduse @Satellite-QE @dosas @omkarkhatavkar @JacobCallahan @lpramuk @jyejare @jameerpathan111 @ColeHiggins2 @rmynar