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: handle empty deployment config yaml file #731

Open
wants to merge 1 commit into
base: main-enterprise
Choose a base branch
from

Conversation

PendaGTP
Copy link
Contributor

@PendaGTP PendaGTP commented Jan 6, 2025

Fixes #730

Changes:

  • Add || {} fallback for yaml.load() results
  • Add || [] fallback for array properties (overridevalidators and configvalidators)
  • Add array validation before iteration

Testing:

  • Verified handling of empty config file
  • Verified handling of null array cases

@PendaGTP
Copy link
Contributor Author

PendaGTP commented Jan 6, 2025

Current implementation: deployment-settings.yml uses defaults when missing but not when empty.

Consider either:

  • using defaults for both missing and empty files
  • documenting that empty files clear defaults

Thoughts?

@HagegeR
Copy link

HagegeR commented Jan 9, 2025

Current implementation: deployment-settings.yml uses defaults when missing but not when empty.

Consider either:

  • using defaults for both missing and empty files
  • documenting that empty files clear defaults

Thoughts?

Shouldn't empty values skip the application of the rule (keep current setting on the repo)?

@PendaGTP
Copy link
Contributor Author

PendaGTP commented Jan 9, 2025

Current implementation: deployment-settings.yml uses defaults when missing but not when empty.
Consider either:

  • using defaults for both missing and empty files
  • documenting that empty files clear defaults

Thoughts?

Shouldn't empty values skip the application of the rule (keep current setting on the repo)?

To clarify, this change is specifically about deployment-config.yml (example here), which is the app's core configuration file. Unlike other settings files, this configuration cannot be overridden.

Regarding the original question about empty vs missing files:
This current implementation applies defaults when the file is missing, but not when the file is empty.

Please let me know if I've missed something or if you need additionnal clarification.

@PendaGTP PendaGTP force-pushed the fix/handle-empty-deployment-settings branch from 41d2889 to 7d1a1b4 Compare January 9, 2025 11:01
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.

Empty deployment-settings.yml cause error
2 participants