Cleanup branch #115

aspacca · 2022-05-16T09:04:54Z

Cleanup

What does this PR do?

Add CF params to easy deploying through IaC
Replace es_index_or_datastream_name in elasticsearch output with datastream

Why is it important?

Checklist

My code follows the style guidelines of this project
~~- [ ] I have commented my code, particularly in hard-to-understand areas~~
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.md

Author's Checklist

[ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

…cloudformation bugs

aspacca · 2022-05-16T09:18:36Z

.internal/aws/cloudformation/macro.yaml

+Description: >
+  Elastic Serverless Forwarder
+
+  SAM Template for the macro, not intended to be deployed on its own


@ravikesarwani
please, let me know if you want to improve the wording of this sentence in order to instruct the users that this specific application is not the one to be deployed

Here's my suggestion around naming the helper macro and the application.
Macro:
Name: helper-macro-elastic-serverless-forwarder
Description:
NOTE: DO NOT DEPLOY
Deploy elastic-serverless-forwarder instead. This is a helper SAM template for the macro and not intended to be deployed on its own.

Application:
Name: helper-application-elastic-serverless-forwarder
Description:
NOTE: DO NOT DEPLOY
Deploy elastic-serverless-forwarder instead. This is a helper SAM template for the application and not intended to be deployed on its own.

I will replace the content, but links are not available in the SAR snippet.
Please let me know if you prefer

Deploy elastic-serverless-forwarder instead.

or

Deploy https://serverlessrepo.aws.amazon.com/applications/eu-central-1/267093732750/elastic-serverless-forwarder instead.

(I would rather use the link to the app in SAR, the one you've provided is region specific)

"Deploy elastic-serverless-forwarder instead." is fine.

.internal/aws/cloudformation/application.yaml

docs/README-AWS.md

elasticmachine · 2022-05-16T09:29:58Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-05-24T02:26:47.021+0000
Duration: 24 min 33 sec

Test stats 🧪

Test	Results
Failed	0
Passed	66
Skipped	0
Total	66

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

elasticmachine · 2022-05-16T09:30:05Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`5/5`)	💚
Files	100.0% (`28/28`)	💚
Classes	100.0% (`28/28`)	💚
Lines	100.0% (`1584/1584`)	💚
Conditionals	100.0% (`596/596`)	💚

endorama · 2022-05-17T09:23:56Z

share/config.py

@@ -290,6 +290,11 @@ def add_output(self, output_type: str, **kwargs: Any) -> None:

        output: Optional[Output] = None
        if output_type == "elasticsearch":
+            if "es_index_or_datastream_name" in kwargs:


Can we link to an issue where this breaking change is detailed and is written the action item of removing this check at a later version?

endorama · 2022-05-17T09:25:54Z

share/config.py

@@ -57,7 +57,7 @@ def __init__(
        self.username = username
        self.password = password
        self.api_key = api_key
-        self.es_index_or_datastream_name = es_index_or_datastream_name
+        self.datastream = datastream


Albeit this is called datastream can still receive an index name, is that correct? So the functionality is still there, we just renamed the setting field.

yes: https://github.com/elastic/elastic-serverless-forwarder/pull/115/files#diff-e30b3f6b54e55786ed08b5bc55aba70c4014a75d87a61054560a945feee77223R293

share/config.py

handlers/aws/replay_trigger.py

ravikesarwani · 2022-05-17T14:27:22Z

Can we put the current image from SAR showing the 3 apps as will be seen by the customers?
That will help be visualize and think through the name and descriptions we should maybe use.

aspacca · 2022-05-18T03:36:15Z

Can we put the current image from SAR showing the 3 apps as will be seen by the customers?

I cannot take the screenshot from SAR before we actually deploy the application.
We can have a patch bump of the version once it's done and added to the readme just for that.

This is a screenshot of my dev deployment (note my name as author):

Not sure about the order the three application areshowed: it seems to be the publication order (first macro helper, then application helper, then the main bundled one), but I remember having seen different order while developing

This is the order when searching for elastic

zmoog

LGTM

Andrea Spacca added 8 commits May 12, 2022 17:20

rename get_account_id_from_lambda_arn to get_account_id_from_arn

e7ee16e

type in changelog

ae73c54

bundle macro and add extra CF params

e56f829

safety checks on macro

91b162e

make the macro more resilient and apply workaround for terraform and …

5d28092

…cloudformation bugs

fix documentation

d353299

changelog

8eb8482

replace es_index_or_datastream_name with datastream

5115e1a

aspacca requested review from zmoog, endorama, girodav, sayden and kaiyan-sheng May 16, 2022 09:04

aspacca self-assigned this May 16, 2022

Andrea Spacca added 2 commits May 16, 2022 18:05

changelog

fc2c66e

use producation application_id for terraform in docs

4589b29

aspacca commented May 16, 2022

View reviewed changes

.internal/aws/cloudformation/application.yaml Show resolved Hide resolved

fix readme

3460fa6

aspacca commented May 16, 2022

View reviewed changes

docs/README-AWS.md Outdated Show resolved Hide resolved

Andrea Spacca added 3 commits May 17, 2022 10:06

improve docs

866f9f6

reference bugs links on IaC notes

caf1365

remove reference to dev artifacts

f12b228

endorama reviewed May 17, 2022

View reviewed changes

kaiyan-sheng reviewed May 17, 2022

View reviewed changes

share/config.py Outdated Show resolved Hide resolved

ravikesarwani reviewed May 17, 2022

View reviewed changes

handlers/aws/replay_trigger.py Outdated Show resolved Hide resolved

use es_datastream_name as config param

b4e7232

aspacca mentioned this pull request May 18, 2022

Remove backward compatibility code for es_index_or_datastream_name in v1.0.0 #116

Closed

template and readme cr fix

d88b5c2

Andrea Spacca added 3 commits May 19, 2022 08:28

use new application name for helpers im cf template

1d8b39b

Merge branch 'main' into cleanup-branch

0a8d635

Merge branch 'main' into cleanup-branch

72d1083

zmoog approved these changes May 24, 2022

View reviewed changes

aspacca merged commit 6cd4b55 into elastic:main May 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup branch #115

Cleanup branch #115

aspacca commented May 16, 2022 •

edited

Loading

aspacca May 16, 2022

ravikesarwani May 17, 2022

aspacca May 18, 2022 •

edited

Loading

ravikesarwani May 18, 2022

elasticmachine commented May 16, 2022 •

edited

Loading

Build stats

Test stats 🧪

elasticmachine commented May 16, 2022

endorama May 17, 2022 •

edited

Loading

aspacca May 18, 2022

endorama May 17, 2022

aspacca May 18, 2022

ravikesarwani commented May 17, 2022

aspacca commented May 18, 2022 •

edited

Loading

zmoog left a comment

Cleanup branch #115

Cleanup branch #115

Conversation

aspacca commented May 16, 2022 • edited Loading

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

aspacca May 16, 2022

Choose a reason for hiding this comment

ravikesarwani May 17, 2022

Choose a reason for hiding this comment

aspacca May 18, 2022 • edited Loading

Choose a reason for hiding this comment

ravikesarwani May 18, 2022

Choose a reason for hiding this comment

elasticmachine commented May 16, 2022 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

elasticmachine commented May 16, 2022

🌐 Coverage report

endorama May 17, 2022 • edited Loading

Choose a reason for hiding this comment

aspacca May 18, 2022

Choose a reason for hiding this comment

endorama May 17, 2022

Choose a reason for hiding this comment

aspacca May 18, 2022

Choose a reason for hiding this comment

ravikesarwani commented May 17, 2022

aspacca commented May 18, 2022 • edited Loading

zmoog left a comment

Choose a reason for hiding this comment

aspacca commented May 16, 2022 •

edited

Loading

aspacca May 18, 2022 •

edited

Loading

elasticmachine commented May 16, 2022 •

edited

Loading

endorama May 17, 2022 •

edited

Loading

aspacca commented May 18, 2022 •

edited

Loading