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

Add IntegrationExecutionContext.stepMetadata and propagate custom config type data #795

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aiwilliams
Copy link
Contributor

Improved the IntegrationStep and IntegrationInvocationConfig type definitions to allow for expressing the type of the IntegrationExecutionConfig that is returned from loadExecutionConfig and passed to executionHandler functions.

This can be a patch release. The type parameters are optional.

@aiwilliams aiwilliams requested a review from a team September 17, 2022 04:18
@aiwilliams aiwilliams self-assigned this Sep 17, 2022
@aiwilliams
Copy link
Contributor Author

  • TInstanceConfig represents the instance configuration and TExecutionConfig represents the result of loadExecutionConfig. Most of the changes are related to propagating this information throughout the code.
  • I thought it worthwhile to rename TConfig and TIntegrationConfig to make them the same value, and figured TInstanceConfig was the clearest choice.
  • I added an additional fixture for an integration that leverages these types while keeping the simpler, existing fixture integration to ensure that we are not required to provide these types when an integration doesn't have custom types.

@aiwilliams aiwilliams changed the title Improve IntegrationStep, IntegrationInvocationConfig types Add IntegrationExecutionContext.stepMetadata and propagate custom config type data Sep 19, 2022
@aiwilliams aiwilliams changed the title Add IntegrationExecutionContext.stepMetadata and propagate custom config type data Add IntegrationExecutionContext.stepMetadata and propagate custom config type data Sep 19, 2022
- Improved type definitions to allow for expressing the type of the
`IntegrationExecutionConfig` that is returned from `loadExecutionConfig` and
passed to `executionHandler` functions.
- Added `IntegrationStepExecutionContext.stepMetadata` to allow
Copy link
Contributor

Choose a reason for hiding this comment

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

What use cases do you imagine needing the stepMetadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The integration I'm working on for generating load on persister/core is going to use step metadata in the executionHandler bound to the step to generate data. After getting some sleep, I realized I can easily reference the step metadata object because it is a module constant in the file where the executionHandler function is defined. It seems natural that the IntegrationStepExecutionContext would have the metadata on it, so I carried on with making it work.

Copy link
Contributor

@VDubber VDubber left a comment

Choose a reason for hiding this comment

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

Thanks for the major improvements here!

Do you believe this will be a non-breaking version?
Can we help with the testing in handful of integrations?

@aiwilliams
Copy link
Contributor Author

Do you believe this will be a non-breaking version?

I believe it should be non-breaking.

Can we help with the testing in handful of integrations?

That will be helpful to coordinate, yes!

I made some other changes last week that worked fine for a graph-* project to adopt (tests/build went fine for the integration) but didn't "just work" when deployed to production: https://jptrone.slack.com/archives/C0174AZA75J/p1663180832691149

I'll be sure to link a local build of these into the jupiter-managed-integration runtime package, but then it will be necessary to deploy an integration that adopts the versions to jupiterone-dev and make sure it works.

@aiwilliams aiwilliams force-pushed the DP-912/execution-config-type-improvements branch 2 times, most recently from 210d267 to f3f68d1 Compare September 21, 2022 17:46
Copy link
Contributor Author

@aiwilliams aiwilliams left a comment

Choose a reason for hiding this comment

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

I’ve done the following:

  • Rebased on main and cleaned up the commits a bit
  • Linked into the integration I’m working on for load testing to be sure it works as desired for executing the integration in the CLI
  • Linked into jupiter-managed-integration to be sure all of its tests are passing and no code changes were necessary

Comment on lines -51 to -54
export type StepExecutionHandlerFunction<
TConfig extends IntegrationInstanceConfig = IntegrationInstanceConfig,
> = ExecutionHandlerFunction<IntegrationStepExecutionContext<TConfig>>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aiwilliams aiwilliams force-pushed the DP-912/execution-config-type-improvements branch from f3f68d1 to d30bac8 Compare September 22, 2022 14:32
Fix stepMetadata field placement and propagate

Clearly there needs to be a better test for this...

Add a test to ensure stepMetadata reaches execution handler
Copy link
Contributor

@austinkelleher austinkelleher left a comment

Choose a reason for hiding this comment

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

Thanks for all of the improvements! I manually linked and tested this with graph-aws and graph-google-cloud, which is how I found a few of the issues.

Are there any documentation improvements that we should consider making?

Step<IntegrationStepExecutionContext<TConfig>>,
export interface StepSpec<
TInstanceConfig extends IntegrationInstanceConfig,
TExecutionConfig extends IntegrationExecutionConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change unless we fall back to IntegrationExecutionConfig as the default. The AWS integration is using StepSpec heavily e.g. https://github.com/JupiterOne/graph-aws/blob/d402343227e24395b57725924dc60ac5a403fe5c/docs/spec/src/accessanalyzer/index.ts#L4

> {
instanceConfig: TConfig;
instanceConfig: TInstanceConfig;
executionConfig: TExecutionConfig;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a breaking change. Should this be optional? Many integrations are using the createMockExecutionContext test function and will complain about the executionConfig property being omitted. https://github.com/search?q=org%3AJupiterOne+%22createMockExecutionContext%22&type=code

TExecutionConfig extends IntegrationExecutionConfig = IntegrationExecutionConfig,
> = CreateMockExecutionContextOptions<TInstanceConfig, TExecutionConfig> &
CreateMockJobStateOptions & {
stepMetadata: StepMetadata;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a breaking change too. stepMetadata is now enforced. createMockStepExecutionContext is used in many integrations: https://github.com/search?q=org%3AJupiterOne+%22createMockStepExecutionContext%22&type=code

I think we will want something like this:

export type CreateMockStepExecutionContextOptions<
  TInstanceConfig extends IntegrationInstanceConfig = IntegrationInstanceConfig,
  TExecutionConfig extends IntegrationExecutionConfig = IntegrationExecutionConfig,
> = CreateMockExecutionContextOptions<TInstanceConfig, TExecutionConfig> &
  CreateMockJobStateOptions & {
    stepMetadata?: StepMetadata;
  };

// ...
// ...

export function createMockStepExecutionContext<
  TInstanceConfig extends IntegrationInstanceConfig = IntegrationInstanceConfig,
  TExecutionConfig extends IntegrationExecutionConfig = IntegrationExecutionConfig,
>(
  options: CreateMockStepExecutionContextOptions<
    TInstanceConfig,
    TExecutionConfig
  > = {
    instanceConfigFields:
      {} as IntegrationInstanceConfigFieldMap<TInstanceConfig>,
  },
): MockIntegrationStepExecutionContext<TInstanceConfig, TExecutionConfig> {
  return {
    ...createMockExecutionContext<TInstanceConfig, TExecutionConfig>(options),
    stepMetadata: options.stepMetadata || ({} as StepMetadata),
    jobState: createMockJobState(options),
  };
}

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.

None yet

3 participants