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 marshalling logic to correctly handle array of aliases #99

Conversation

ElizabethMRichardson
Copy link
Contributor

@ElizabethMRichardson ElizabethMRichardson commented Mar 12, 2024

tldr

We want to handle the situation where the result of a JS expression returns an array.

The changes

  • Update MarshalEntries to check if the result from expr.EvaluateSingleValue is empty, rather than checking for an error
  • Update the logic in MarshalEntries that is dealing with arrays to use expr.EvaluateArray rather than expr.EvaluateSingleValue (an existing bug from before cel -> js we believe)

Some extra context if needed

We want to be able to handle marshalling aliases in the situation where catalog entries have different numbers of aliases. Currently you can set the alias source like: aliases: ['$.aliases[0]', '$.aliases[1]','$.aliases[2]'], but this doesn't work in the situation above where the lengths could differ.
For example, a jsonnet with:

      sources: [
        {
          inline: {
            entries: [
              {
                name: 'Low',
                external_id: 'low',
                description:
                  'Non-essential features that customers are happy to wait for PR to fix issues with.',
                aliases: ['low'],
              },
              {
                name: 'Medium',
                external_id: 'medium',
                description:
                  'Needs immediate triaging but may not require immediate response. This is the default criticality if left unspecified.',
                aliases: ['medium', 'med', 'mid'], 
              },
              {
                name: 'High',
                external_id: 'high',
                description:
                  'Immediate response required as this impacts a critical part of the customer experience.',
                aliases: ['high', 'max'],
              },
            ],
          },
        },
      ],

We want to be able to set the source like:

source: {
            name: '$.name',
            external_id: '$.external_id',
            aliases: ['$.aliases'],
          },

and have this be handled correctly.

@ElizabethMRichardson ElizabethMRichardson changed the title Allow handling of array of aliases when source set to ['$.aliases'] Allow handling of array of aliases when length is unknown Mar 13, 2024
@ElizabethMRichardson ElizabethMRichardson changed the title Allow handling of array of aliases when length is unknown Fix marshalling logic to correctly handle array of aliases Mar 13, 2024
@ElizabethMRichardson
Copy link
Contributor Author

to do: tests!

@ElizabethMRichardson ElizabethMRichardson force-pushed the lizrichardson/resp-2845-allow-catalog-importer-to-handle-array-of-aliases branch from fa848d1 to 179bec1 Compare March 13, 2024 15:29
@ElizabethMRichardson ElizabethMRichardson force-pushed the lizrichardson/resp-2845-allow-catalog-importer-to-handle-array-of-aliases branch from 179bec1 to 5abf4dd Compare March 13, 2024 15:30
Copy link
Contributor

@pipt pipt 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 to me!

@ElizabethMRichardson ElizabethMRichardson merged commit 45f186f into master Mar 13, 2024
1 check passed
@ElizabethMRichardson ElizabethMRichardson deleted the lizrichardson/resp-2845-allow-catalog-importer-to-handle-array-of-aliases branch March 13, 2024 15:42
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