Skip to content

Fix artificial delay override #5035

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

Merged
merged 4 commits into from
Apr 22, 2025
Merged

Conversation

mapno
Copy link
Contributor

@mapno mapno commented Apr 21, 2025

What this PR does:

This PR contains two changes:

  1. Correctly handles conversion of ingestion_artificial_delay field between config formats
  2. Fixes other conversion issues and adds comprehensive conversion tests

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Copy link
Contributor

@javiermolinar javiermolinar left a comment

Choose a reason for hiding this comment

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

Nice!


// Skip certain fields that can be zero in valid configs
switch fieldName {
case "IngestionArtificialDelay": // Can legitimately be nil
Copy link
Contributor

@javiermolinar javiermolinar Apr 22, 2025

Choose a reason for hiding this comment

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

what do you think about creating a static list of fieldNames. That way it would be easier to add new nil values to be skipped:

	var skip = []string{"IngestionArtificialDelay"}

	if slices.Contains(skip, fieldName) {
		continue
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now it's just that field, but sounds good 👍

@mapno mapno enabled auto-merge (squash) April 22, 2025 10:56
@mapno mapno merged commit 5482494 into grafana:main Apr 22, 2025
15 checks passed
@mapno mapno deleted the fix-artificial-delay-override branch April 22, 2025 13:37
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.

2 participants