feat(provider/aws): Functions (listing and searching) #7536

sidmuls · 2019-10-16T17:04:12Z

Issue: https://github.com/spinnaker/spinnaker/issues/4921Issue
Summary:
Add functions support to Spinnaker UI. This is a net functionality that we've been working on for almost a year to this point. The initial backend work was done like three months back.
From then we've worked on creating the UI components with guidance from the Netflix UX team.

Cloud Provider(s): provider/AWS
Environment:
This worked on user's local environment and also through deploying docker images to kubernetes cluster.

Feature Area: @spinnaker/ui-ux-team
Description:
CRUD functionality for managing lambda functions, With this new we must be able to create, update and delete lambda lambda functions through Spinnaker UI
We must also be able to attach target groups to lambda functions using Spinnaker UI.
Additional Details:
Clouddriver backend details: https://github.com/spinnaker/clouddriver/tree/master/clouddriver-lambda

spinnakerbot · 2019-10-16T17:05:23Z

The following commits need their title changed:

c7d0768: .
b340011: feat(provider/aws):Function listing and searching functionality

Please format your commit title into the form:

<type>(<scope>): <subject>, e.g. fix(kubernetes): address NPE in status check

This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here.

@sidmuls

This change adds a new tab for functions and facilitates listing existing functions and searching functions listed. Corresponding provider specific changes are included in this change for AWS. Authors: @sidmuls and @nabebe

@sidmuls

This change adds a new tab for functions and facilitates listing existing functions and searching functions listed. Corresponding provider specific changes are included in this change for AWS. Authors: @sidmuls and @nabebe

caseyhebebrand · 2019-10-16T17:45:19Z

app/scripts/modules/amazon/src/domain/IAmazonFunction.ts

+  functionName: string;
+  publish: boolean;
+  description: string;
+  tags: [{}];


Is it possible to use a more explicit type or generics for the tags?

I had thought of having tags: [{[key: string] : string}]
But since tags can be empty, the following error is thrown:

Property '0' is missing in type '[]' but required in type '[{ [key: string]: string; }]'.

Could you please suggest a better way?

It looks like {} is also equiavalent to { [key: string]: any }, so {} may be the best option unless there is a constant, explicit type when there is a tag present

caseyhebebrand · 2019-10-16T17:45:20Z

app/scripts/modules/amazon/src/domain/IAmazonFunction.ts

+  tags: [{}];
+  memorySize: number;
+  timeout: number;
+  envVariables: {};


If envVariables will not be empty it would be stronger to make the type { [key: string]: any }.

it can be empty if no env vars are set on the function.

got it, then feel free to leave it as is.

caseyhebebrand · 2019-10-16T17:45:25Z

app/scripts/modules/core/src/function/Function.tsx

+  functionDef: IFunction;
+}
+
+export class Function extends React.Component<IFunctionProps> {


Can we make this a functional component since it is stateless and presentational only?

alanmquach · 2019-10-16T20:18:28Z

app/scripts/modules/core/src/function/FunctionDetails.tsx

+export interface IFunctionDetailsProps extends IOverridableProps {}
+
+@Overridable('function.details')
+export class FunctionDetails extends React.Component<IFunctionDetailsProps> {


Unfortunately, it looks like our @Overridable annotation can only decorate class components.

https://github.com/spinnaker/deck/blob/master/app/scripts/modules/core/src/overrideRegistry/Overridable.tsx#L36-L40

At least in its current form. 😞

Thanks for pointing this out. A PureComponent could be a viable alternative in the current form, I would have to look at the differences between the two interfaces in react.

I think this is fine as-is. This component is effectively a placeholder and hopefully never gets rendered.

sidmuls · 2019-10-16T17:57:49Z

app/scripts/modules/core/src/function/Functions.tsx

+
+export class Functions extends React.Component<IFunctionsProps, IFunctionsState> {
+  private groupsUpdatedListener: Subscription;
+  private functionsRefreshUnsubscribe: () => any;


the definition of that function maps to

this.functionsRefreshUnsubscribe = app .getDataSource('functions') .onRefresh(null, () => this.updateFunctionGroups());

the onRefresh returns any

caseyhebebrand · 2019-10-16T17:45:30Z

app/scripts/modules/core/src/function/Functions.tsx

+  };
+
+  public render(): React.ReactElement<Functions> {
+    const groupings = this.state.initialized ? (


Groupings should be its own component, especially since it has already been abstracted away from the rest of the presentational layer.

@caseyhebebrand

…tion component. This is a refactoring change as requested by reviewer @caseyhebebrand Author: @sidmuls

@caseyhebebrand

This change is for refactoring requested by reviewer @caseyhebebrand. Author: @sidmuls

caseyhebebrand · 2019-10-17T00:00:05Z

app/scripts/modules/core/src/function/FunctionGroupings.tsx

+    super(props);
+
+    const { groups } = props;
+    this.state = {


You can just directly use the props instead of assigning groups to the state. Especially since you are never mutating the groups within the component. It will re-render anyway if the groups in props are updated.

Pushed the change :)

@caseyhebebrand

…ings. This change is a refactoring requested by @caseyhebebrand Author: @sidmuls

caseyhebebrand · 2019-10-17T00:13:53Z

LGTM, thanks for addressing the comments

…etrying tests on TravisCI

christopherthielen

Looks pretty good! I just have a few requests and questions.

christopherthielen · 2019-10-17T22:18:34Z

app/scripts/modules/core/src/domain/IFunction.ts

+  description?: string;
+  name?: string;
+  functionName?: string;
+  provider?: string;


What is provider?

some providers may add the 'cloud provider' field with the key 'provider'
it was to accomodate for that discrepancy from the backend.
but we can probably remove it and enforce the filed key to be 'cloudProvider'
I have removed it in the next push.

christopherthielen · 2019-10-17T22:24:11Z

app/scripts/modules/core/src/function/FunctionDetails.tsx

+export interface IFunctionDetailsProps extends IOverridableProps {}
+
+@Overridable('function.details')
+export class FunctionDetails extends React.Component<IFunctionDetailsProps> {


I think this is fine as-is. This component is effectively a placeholder and hopefully never gets rendered.

christopherthielen · 2019-10-17T22:30:16Z

app/scripts/modules/core/src/function/filter/FunctionFilterModel.ts

+
+  constructor() {
+    this.asFilterModel = FilterModelService.configureFilterModel(this as any, filterModelConfig);
+    FilterModelService.registerRouterHooks(this.asFilterModel, '**.application.insight.loadBalancers.**');


'**.application.insight.loadBalancers.**'

this doesn't seem right

thank you for pointing this out :)
fixed in the next push.

christopherthielen · 2019-10-17T22:32:23Z

app/scripts/modules/core/src/function/filter/FunctionFilterService.ts

+
+  constructor() {}
+
+  private addSearchFields(functionDef: IFunction): void {


I'm wondering if this should be done in the transformer?

I thought this fits here because it is specific to filtering.
I used loadbalancer module as reference.

christopherthielen · 2019-10-17T22:37:16Z

app/scripts/modules/core/src/function/filter/FunctionFilterService.ts

+
+    // sort groups in place so Angular doesn't try to update the world
+    FunctionState.filterModel.asFilterModel.groups.sort((a, b) => {
+      if (a.heading < b.heading) {


return a.heading.localeCompare(b.heading)

christopherthielen · 2019-10-17T22:54:25Z

app/scripts/modules/core/src/function/filter/FunctionFilterService.ts

+    const functions = this.filterFunctionsForDisplay(application.functions.data);
+    const grouped = groupBy(functions, 'account');
+
+    forOwn(grouped, (group, account) => {


would love to see this section extracted to a pure function, then some tests written against it. It's difficult to read and the intentions are not completely clear.

added in the next push.

christopherthielen · 2019-10-17T23:36:09Z

app/scripts/modules/core/src/function/filter/FunctionFilterService.ts

+      .value();
+  }
+
+  private diffSubgroups(oldGroups: IFunctionGroup[], newGroups: IFunctionGroup[]): void {


would like to see some tests for this function

I figured we don't need this method altogether.
removed it in next push.

christopherthielen · 2019-10-17T23:54:08Z

app/scripts/modules/core/src/function/function.read.service.ts

+  public loadFunctions(applicationName: string): IPromise<IFunctionSourceData[]> {
+    return API.one('applications', applicationName)
+      .all('functions')
+      .withParams({ region: 'us-west-2' })


hard coded region

removed in next push :)

christopherthielen · 2019-10-17T23:57:41Z

app/scripts/modules/core/src/function/function.states.ts

+  (applicationStateProvider: ApplicationStateProvider, stateConfigProvider: StateConfigProvider) => {
+    const functionDetails: INestedState = {
+      name: 'functionDetails',
+      url: '/functionDetails/:provider/:account/:region/:vpcId/:name',


we can't uniquely target a function without a vpcId?

we can. removed it in my next push. Thank you for pointing this out :)

@christopherthielen

…anges as requested by @christopherthielen Author: @sidmuls

christopherthielen · 2019-10-25T00:59:21Z

app/scripts/modules/core/src/function/function.read.service.ts

+
+  private normalizeFunction(functionDef: IFunctionSourceData): IPromise<IFunction> {
+    return this.functionTransformer.normalizeFunction(functionDef).then((fn: IFunction) => {
+      fn.cloudProvider = fn.cloudProvider || 'aws';


Does the backend ever return functions without a cloudProvider field? This seems a little odd since I assume the backend always returns 'aws' for Lambdas?

christopherthielen · 2019-10-25T00:59:57Z

app/scripts/modules/core/src/function/function.states.ts

+              name: $stateParams.name,
+              accountId: $stateParams.account,
+              region: $stateParams.region,
+              vpcId: $stateParams.vpcId,


remove this line

christopherthielen

A couple of minor things to address, but those can be in a subsequent pr.

This reverts commit 86a365b.

#7567) This reverts commit 86a365b.

d71daa5 fix(core/pipeline): fully re-render list of trigger configs after a delete (spinnaker#7571) 629a98f fix(core/pipeline): make revision dropdown usable, layout tweaks (spinnaker#7569) ca176fc feat(provider/aws): Functions (listing and searching) (spinnaker#7568) e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (spinnaker#7536)" (spinnaker#7567) 114303a fix(kubernetes): disable project cluster filtration by stack/detail (spinnaker#7562) 86a365b feat(provider/aws): Functions (listing and searching) (spinnaker#7536) 6236a9f fix(core/pipeline): make UX less bad when a pipeline stage never happened (spinnaker#7563)

c2bbf20 feat(rosco): Allow roscoMode per stage/execution (spinnaker#7564) ca176fc feat(provider/aws): Functions (listing and searching) (spinnaker#7568) e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (spinnaker#7536)" (spinnaker#7567) 86a365b feat(provider/aws): Functions (listing and searching) (spinnaker#7536)

d71daa5 fix(core/pipeline): fully re-render list of trigger configs after a delete (#7571) 629a98f fix(core/pipeline): make revision dropdown usable, layout tweaks (#7569) ca176fc feat(provider/aws): Functions (listing and searching) (#7568) e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (#7536)" (#7567) 114303a fix(kubernetes): disable project cluster filtration by stack/detail (#7562) 86a365b feat(provider/aws): Functions (listing and searching) (#7536) 6236a9f fix(core/pipeline): make UX less bad when a pipeline stage never happened (#7563)

c2bbf20 feat(rosco): Allow roscoMode per stage/execution (#7564) ca176fc feat(provider/aws): Functions (listing and searching) (#7568) e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (#7536)" (#7567) 86a365b feat(provider/aws): Functions (listing and searching) (#7536)

…ker#7536)" (spinnaker#7567) This reverts commit 86a365b.

d71daa5 fix(core/pipeline): fully re-render list of trigger configs after a delete (spinnaker#7571) 629a98f fix(core/pipeline): make revision dropdown usable, layout tweaks (spinnaker#7569) ca176fc feat(provider/aws): Functions (listing and searching) (spinnaker#7568) e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (spinnaker#7536)" (spinnaker#7567) 114303a fix(kubernetes): disable project cluster filtration by stack/detail (spinnaker#7562) 86a365b feat(provider/aws): Functions (listing and searching) (spinnaker#7536) 6236a9f fix(core/pipeline): make UX less bad when a pipeline stage never happened (spinnaker#7563)

c2bbf20 feat(rosco): Allow roscoMode per stage/execution (spinnaker#7564) ca176fc feat(provider/aws): Functions (listing and searching) (spinnaker#7568) e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (spinnaker#7536)" (spinnaker#7567) 86a365b feat(provider/aws): Functions (listing and searching) (spinnaker#7536)

@sidmuls

* . * feat(provider/aws): Function listing and searching functionality This change adds a new tab for functions and facilitates listing existing functions and searching functions listed. Corresponding provider specific changes are included in this change for AWS. Authors: @sidmuls and @nabebe * feat(provider/aws): Function listing and searching functionality This change adds a new tab for functions and facilitates listing existing functions and searching functions listed. Corresponding provider specific changes are included in this change for AWS. Authors: @sidmuls and @nabebe * refactor(provider/aws): Converting Function class component into function component. This is a refactoring change as requested by reviewer @caseyhebebrand Author: @sidmuls * refactor(provider/aws): Refactoring groupings into a separate component. This change is for refactoring requested by reviewer @caseyhebebrand. Author: @sidmuls * refactor(provider/aws): Removing unnecessary state from FunctionGroupings. This change is a refactoring requested by @caseyhebebrand Author: @sidmuls * refactor(provider/aws): removing extra line. Tthis is actually for retrying tests on TravisCI * refactor(provider/aws): Adding tests for FunctionFilterService and changes as requested by @christopherthielen Author: @sidmuls * refactor(provider/aws): removing unnecessary field from IFunction

…ker#7536)" (spinnaker#7567) This reverts commit 86a365b.

d71daa5 fix(core/pipeline): fully re-render list of trigger configs after a delete (spinnaker#7571) 629a98f fix(core/pipeline): make revision dropdown usable, layout tweaks (spinnaker#7569) ca176fc feat(provider/aws): Functions (listing and searching) (spinnaker#7568) e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (spinnaker#7536)" (spinnaker#7567) 114303a fix(kubernetes): disable project cluster filtration by stack/detail (spinnaker#7562) 86a365b feat(provider/aws): Functions (listing and searching) (spinnaker#7536) 6236a9f fix(core/pipeline): make UX less bad when a pipeline stage never happened (spinnaker#7563)

c2bbf20 feat(rosco): Allow roscoMode per stage/execution (spinnaker#7564) ca176fc feat(provider/aws): Functions (listing and searching) (spinnaker#7568) e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (spinnaker#7536)" (spinnaker#7567) 86a365b feat(provider/aws): Functions (listing and searching) (spinnaker#7536)

.

c7d0768

sidmuls mentioned this pull request Oct 16, 2019

Functions: List and Search functionality #7533

Closed

sidmuls force-pushed the master branch from b340011 to c15d097 Compare October 16, 2019 17:06

sidmuls force-pushed the master branch from c15d097 to 0dbd5f2 Compare October 16, 2019 17:07

sidmuls added 2 commits October 16, 2019 10:11

Merge branch 'master' of https://github.com/sidmuls/deck

4caa90c

caseyhebebrand reviewed Oct 16, 2019

View reviewed changes

sidmuls added 2 commits October 16, 2019 15:57

refactor(provider/aws): Converting Function class component into func…

0d04734

…tion component. This is a refactoring change as requested by reviewer @caseyhebebrand Author: @sidmuls

refactor(provider/aws): Refactoring groupings into a separate component.

aa9cdc0

This change is for refactoring requested by reviewer @caseyhebebrand. Author: @sidmuls

caseyhebebrand reviewed Oct 17, 2019

View reviewed changes

refactor(provider/aws): Removing unnecessary state from FunctionGroup…

1cbb844

…ings. This change is a refactoring requested by @caseyhebebrand Author: @sidmuls

refactor(provider/aws): removing extra line. Tthis is actually for r…

d1b43d2

…etrying tests on TravisCI

christopherthielen reviewed Oct 18, 2019

View reviewed changes

sidmuls added 2 commits October 22, 2019 12:09

refactor(provider/aws): Adding tests for FunctionFilterService and ch…

d96a32b

…anges as requested by @christopherthielen Author: @sidmuls

refactor(provider/aws): removing unnecessary field from IFunction

ab69450

This was referenced Oct 23, 2019

REQUEST: New Reviewer status for caseyhebebrand spinnaker/governance#55

Closed

REQUEST: New Approver status for caseyhebebrand spinnaker/governance#56

Closed

christopherthielen reviewed Oct 25, 2019

View reviewed changes

christopherthielen self-requested a review October 25, 2019 01:02

christopherthielen approved these changes Oct 25, 2019

View reviewed changes

christopherthielen added the ready to merge Reviewed and ready for merge label Oct 25, 2019

mergify bot added the auto merged Merged automatically by a bot label Oct 25, 2019

Merge branch 'master' into master

6ca3d99

mergify bot merged commit 86a365b into spinnaker:master Oct 25, 2019

spinnakerbot added the target-release/1.17 label Oct 25, 2019

plumpy added a commit that referenced this pull request Oct 25, 2019

Revert "feat(provider/aws): Functions (listing and searching) (#7536)"

76513b0

This reverts commit 86a365b.

plumpy mentioned this pull request Oct 25, 2019

Revert "feat(provider/aws): Functions (listing and searching)" #7567

Merged

mergify bot pushed a commit that referenced this pull request Oct 25, 2019

Revert "feat(provider/aws): Functions (listing and searching) (#7536)" (

e49ffaf

#7567) This reverts commit 86a365b.

christopherthielen mentioned this pull request Oct 28, 2019

Bump package core to 0.0.427 and docker to 0.0.48 and amazon to 0.0.220 #7573

Merged

Jammy-Louie pushed a commit to pivotal/deck that referenced this pull request Nov 8, 2019

Revert "feat(provider/aws): Functions (listing and searching) (spinna…

16f43cc

…ker#7536)" (spinnaker#7567) This reverts commit 86a365b.

yunzhangit pushed a commit to yunzhangit/deck that referenced this pull request Mar 28, 2021

Revert "feat(provider/aws): Functions (listing and searching) (spinna…

ded4e5d

…ker#7536)" (spinnaker#7567) This reverts commit 86a365b.


		constructor() {}

		private addSearchFields(functionDef: IFunction): void {

feat(provider/aws): Functions (listing and searching) #7536

feat(provider/aws): Functions (listing and searching) #7536

Conversation

sidmuls commented Oct 16, 2019

spinnakerbot commented Oct 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caseyhebebrand commented Oct 17, 2019

christopherthielen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidmuls Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopherthielen left a comment

Choose a reason for hiding this comment

sidmuls Oct 22, 2019 •

edited

Loading