fix/linting for percentage based values

[bmiguel-teixeira]

This PR fixes the PDB validation to include support for % based values.

OldBehavior:
maxUnavailable was set to 10% was failing since the field is IntOrString. The IntVal defaults to 0 when the field is of String Values, thus causing false positives.

NewBehaviour:
int and string values are taken into account, parsed into a single integer value for validation.

[janisz]

@fahlmant PTAL

[jholecek-rh]

Since the MR is importing k8s.io/apimachinery/pkg/util/intstr, wouldn't it be better to refactor the code to use higher-level functions offered by that package, e.g. getIntOrPercentValueSafely?

[tremes]

If the first condition is satisfied then the second one will fail with panic.

[bmiguel-teixeira]

done.

[tremes]

Thank you

[tremes]

I think this is "undesired" diagnostic. I mean we would like to report when MaxUnavailable is 0, otherwise do nothing.

[bmiguel-teixeira]

still have a bit of mixed feeling on this one but a "missing" maxUnavailable is valid. skipping validation on nil. please take a look

[tremes]

yes it's valid. Thanks. Looks good.

[mklika]

Hi @bmiguel-teixeira!

My name is Martin and I work at Red Hat along with Tomas. We're quite eager to see these changes merged as we bellieve PDB check could be quite helpful for our SRE organization.

Would you mind letting me know your estimate in terms of when would you have time look at comments from Tomas and fixing the code?

I'd like to also offer our help finishing this up should it be your preference due to other priorities you might have.

Many thanks in advance for letting me know

Martin Klika

[bmiguel-teixeira]

Hi @mklika

Thanks for the heads up. Changes & fixes submitted. Please review

[tremes]

Looks good to me. Thank you @bmiguel-teixeira .

[janisz]

How about converting this tests to table driven tests?

func (p *PDBTestSuite) TestPDB() {
	testCases := []struct {
		Name           string
		Message        string
		MaxUnavailable intstr.IntOrString
	}{
		{
			Name:           "Invalid Value String",
			Message:        "maxUnavailable has invalid value [invalid-value]",
			MaxUnavailable: intstr.FromString("invalid-value"),
		},
		{
			Name:           "Invalid Value String",
			Message:        "maxUnavailable has invalid value [0]",
			MaxUnavailable: intstr.FromString("0"),
		},
		{
			Name:           "One As String Percentage",
			MaxUnavailable: intstr.FromString("1%"),
		},
		{
			Name:           "One As Integer",
			MaxUnavailable: intstr.FromInt(1),
		},
		{
			Name:           "Zero As String Percentage",
			Message:        "MaxUnavailable is set to 0",
			MaxUnavailable: intstr.FromString("0%"),
		},
		{
			Name:           "Zero As Integer",
			Message:        "MaxUnavailable is set to 0",
			MaxUnavailable: intstr.FromInt(0),
		},
	}

	for _, tc := range testCases {
		tc := tc
		p.Run(tc.Name, func() {
			p.ctx.AddMockPodDisruptionBudget(p.T(), "test-pdb")
			p.ctx.ModifyPodDisruptionBudget(p.T(), "test-pdb", func(pdb *pdbV1.PodDisruptionBudget) {
				pdb.Spec.MaxUnavailable = &tc.MaxUnavailable
			})

			expected := map[string][]diagnostic.Diagnostic{}
			if tc.Message != "" {
				expected = map[string][]diagnostic.Diagnostic{
					"test-pdb": {{Message: tc.Message}},
				}
			}

			p.Validate(p.ctx, []templates.TestCase{
				{
					Param:                    params.Params{},
					Diagnostics:              expected,
					ExpectInstantiationError: false,
				},
			})
		})
	}
}

[bmiguel-teixeira]

make sense. done