SALTO-7399: advanced fields references #7209
Conversation
IdoZakk
force-pushed
the
7399advancedFields
branch
2 times, most recently
from
35ba037 to
76b7fe0
Compare
IdoZakk
force-pushed
the
7399advancedFields
branch
3 times, most recently
from
44a622f to
4eb7e8d
Compare
IdoZakk
force-pushed
the
7399advancedFields
branch
from
4eb7e8d to
7f89b0f
Compare
|
|
||
| const CUSTOM_FIELD_PATTERN = /(customfield_\d+)/ | ||
|
|
||
| const referenceCustomFields = ( | ||
| text: string, | ||
| fieldInstancesById: Map<string, InstanceElement>, | ||
| enableMissingReferences: boolean, |
There was a problem hiding this comment.
IMO it is worth using named parameters
There was a problem hiding this comment.
For three distinct types I tend to keep as parameters, but will do
| import { InstanceElement, Value } from '@salto-io/adapter-api' | ||
| import { AutomationInstance, isAutomationInstance } from './smart_values/smart_value_reference_filter' | ||
|
|
||
| const FIELDS_TO_WALK_ON = (): string[] => ['trigger', 'components'] |
There was a problem hiding this comment.
IMO it should be a constant and not a function
There was a problem hiding this comment.
This is something I learned from Seggev, they use it in Netsuite.
As a constant array can be changed (it is not really a constant) when creating a constant array if you use a function the original array cannot be changed
packages/jira-adapter/src/filters/automation/walk_on_automation.ts
Outdated
Show resolved
Hide resolved
|
|
||
| /** | ||
| * Convert field values into references, based on predefined rules. | ||
| */ | ||
|
|
||
| const noReferencesTypes = [PROJECT_COMPONENT_TYPE, FIELD_CONFIGURATION_TYPE_NAME] | ||
| const NO_REFERENCES_TYPES = (): string[] => [PROJECT_COMPONENT_TYPE, FIELD_CONFIGURATION_TYPE_NAME] |
There was a problem hiding this comment.
why do we need it as a function?
I agree that it should be a constant
There was a problem hiding this comment.
Same as I answered in the previous comment
| const instances = elements.filter(isInstanceElement) | ||
| const fieldInstances = instances.filter(instance => instance.elemID.typeName === FIELD_TYPE_NAME) | ||
| const fieldInstancesById = new Map( | ||
| fieldInstances.map(instance => [instance.value.id, instance] as [string, InstanceElement]), |
There was a problem hiding this comment.
we can filter by _.isString(instance.value.id) to make sure it is legit
and why not use Record as we usually do?
There was a problem hiding this comment.
It was already a map, did not want to refactor (I had a long debate once with Alon about Record vs Map...)
packages/jira-adapter/test/filters/fields/reference_to_fields.test.ts
Outdated
Show resolved
Hide resolved
| `Failed to clean older e2e changes: ${allRemovalErrors.map(e => safeJsonStringify(e)).join(', ')}`, | ||
| ) | ||
| } | ||
| const allRemovalErrors = await deployChanges([allOssCreatedElements], () => true) // do not fail on errors |
There was a problem hiding this comment.
can you explain this change?
There was a problem hiding this comment.
This is a part of the E2E where we clean old instances that were not deleted due to aborted E2E. We had problems with DC so we omitted it, but there is no reason as we do not fail the E2E if this stage fails
| @@ -67,6 +66,9 @@ const excludedTypes = [ | |||
| const nullProgressReporter: ProgressReporter = { | |||
| reportProgress: () => {}, | |||
| } | |||
|
|
|||
| const deleteElementsAtTheEnd = true // use for debugging | |||
There was a problem hiding this comment.
is it a leftover?
I dont understand its purpose because it is always truth
There was a problem hiding this comment.
No, I changed the comment to make it clearer
| const fieldInstances = instances.filter(instance => instance.elemID.typeName === FIELD_TYPE_NAME) | ||
| const fieldInstancesById = new Map( | ||
| fieldInstances.map(instance => [instance.value.id, instance] as [string, InstanceElement]), | ||
| fieldInstances | ||
| .filter(instance => typeof instance.value.id === 'string') |
There was a problem hiding this comment.
nit:
I guess you prefer is that way, but IMO _.isString is cleaner
There was a problem hiding this comment.
I tend to go with a natural solution over a package one
| @@ -42,7 +43,7 @@ const addWalkOnReferences = (elements: Element[], config: JiraConfig): void => { | |||
| const filter: FilterCreator = ({ config }) => ({ | |||
| name: 'fieldReferencesFilter', | |||
| onFetch: async elements => { | |||
| addWalkOnReferences(elements, config) | |||
| addWalkOnReferences(elements.filter(isInstanceElement), config) | |||
There was a problem hiding this comment.
nit:
you can save the instances in a variable before, because you filter by isInstanceElement again in line 53
Added support for advanced fields reference
Required some refactoring, made that in a different commit.
I also fixed a problem (missing functionality) in ScriptRunner where there were no missing references
(also a small bug in the E2E)
Noise reduction (also for scriptRunner)
Release Notes:
Jira Adapter:
User Notifications:
Jira Adapter: