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

Adjust aws package to use input groups #767

Merged
merged 23 commits into from
Jun 29, 2021
Merged

Adjust aws package to use input groups #767

merged 23 commits into from
Jun 29, 2021

Conversation

kaiyan-sheng
Copy link
Contributor

@kaiyan-sheng kaiyan-sheng commented Mar 8, 2021

What does this PR do?

This PR is to modify current aws package to use input groups in order to add icons for each service to support granular search.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.

Related issues

Screenshots

Screen Shot 2021-05-27 at 11 47 12 AM

@kaiyan-sheng kaiyan-sheng self-assigned this Mar 8, 2021
@elasticmachine
Copy link

elasticmachine commented Mar 8, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Branch indexing

  • Start Time: 2021-06-29T22:28:59.179+0000

  • Duration: 15 min 35 sec

  • Commit: 1795121

Test stats 🧪

Test Results
Failed 0
Passed 247
Skipped 0
Total 247

Trends 🧪

Image of Build Times

Image of Tests

@andresrc andresrc added the Team:Integrations Label for the Integrations team label Mar 8, 2021
@sorantis
Copy link

In its current state the manifest.yml is missing the categories section both package wide and service specific. Are we planning to add these?

@kaiyan-sheng
Copy link
Contributor Author

Update: waiting on elastic/elastic-package#282 before working on this PR again

@kaiyan-sheng
Copy link
Contributor Author

In its current state the manifest.yml is missing the categories section both package wide and service specific. Are we planning to add these?

@sorantis Yes! I think package wide categories is already defined in manifest.yml. I will add the service specific ones in next commit! Thanks!!!

@sorantis
Copy link

yes, my bad, I looked at the diff only, which hid the existing categories.

@kaiyan-sheng kaiyan-sheng marked this pull request as ready for review March 23, 2021 11:10
@elasticmachine
Copy link

Pinging @elastic/integrations (Team:Integrations)

@mtojek
Copy link
Contributor

mtojek commented Mar 23, 2021

@kaiyan-sheng I assume we can't merge this package yet, as other parties don't support it yet? I mean Kibana and Package Registry. I'm not sure what would be implications of merging it now :)

@kaiyan-sheng
Copy link
Contributor Author

@kaiyan-sheng I assume we can't merge this package yet, as other parties don't support it yet? I mean Kibana and Package Registry. I'm not sure what would be implications of merging it now :)

@mtojek Yeah I just tested the AWS integration with this PR and I definitely need to work on package registry first before this PR can get merged.

@mtojek
Copy link
Contributor

mtojek commented Mar 23, 2021

Maybe mark this PR as draft to prevent from accidental merge :)

@kaiyan-sheng kaiyan-sheng marked this pull request as draft March 23, 2021 12:51
@kaiyan-sheng
Copy link
Contributor Author

@mtojek Done, back in draft mode! Thanks!

@sorantis
Copy link

The Meta elastic/package-spec#144 indicates that package registry changes have dependency on AWS package. Is it the other way around? Also, what should be done to unblock changes in Kibana?

@kaiyan-sheng
Copy link
Contributor Author

@sorantis I'm currently working on changes in package-registry elastic/package-registry#685 to unblock Kibana work. I also pointed this draft PR to @jen-huang and hopefully combining both draft PRs together will help.

@jen-huang
Copy link
Contributor

Hi all, I'm starting to assess Kibana work using the in-flight changes to AWS package in this PR along with the package registry changes. I have a question regarding the new package-level vars field introduced here:

vars:
- name: shared_credential_file

I know we introduced support for package-level vars a little while ago in the package spec PR, but AFAIK supporting this on the Kibana side was not scoped for phase 1, so I was wondering if we can pull out that change from this PR and put those vars back at the (I think) input level?

@sorantis What do you think? If Kibana should support package-level vars as part of phase 1, can you provide some guidance on how they fit into the existing policy editor UI?

@kaiyan-sheng
Copy link
Contributor Author

@jen-huang My understanding is, for package-level vars, I would expect to see them under current Integration settings. So basically moving Credential Profile Name and Advanced options from under Settings per logs/metrics up a level.
Screen Shot 2021-03-31 at 4 18 58 PM

@jen-huang
Copy link
Contributor

Thanks @kaiyan-sheng, I think that makes sense. I've added supporting package-level vars to the Kibana scope for phase 1: elastic/kibana#93315

@kaiyan-sheng
Copy link
Contributor Author

Hi @kaiyan-sheng, please add input_groups as part of these changes too. Without this field, Kibana won't know what to display in the UI. For more context: elastic/package-registry#685 (comment)

@jen-huang Done, please let me know if it works for you! Thank you!!

@mtojek
Copy link
Contributor

mtojek commented May 20, 2021

@kaiyan-sheng Do input groups need a newer stack (7.14.0-SNAPSHOT)? If so, we need to bump it out in elastic-package. It's not a problem at all, but just asking in advance ;)

@kaiyan-sheng
Copy link
Contributor Author

kaiyan-sheng commented May 24, 2021

Thanks for pointing this out @mtojek ! Yes we do need to limit the stack version. I'm thinking to change the kibana.version in manifest.yml to be ^7.14.0. Let's wait till @jen-huang 's PR get merged first before bumping the version here.

@jen-huang
Copy link
Contributor

FYI elastic/kibana#99866 is just the first part of the Kibana work, there is a second part to come :)

@kaiyan-sheng kaiyan-sheng marked this pull request as ready for review June 22, 2021 13:56
@kaiyan-sheng kaiyan-sheng requested a review from ChrsMark June 22, 2021 21:24
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

lgtm, however conflicts should be resolved

@kaiyan-sheng kaiyan-sheng requested a review from ChrsMark June 29, 2021 15:52
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

:shipit:

@kaiyan-sheng kaiyan-sheng merged commit af9597a into elastic:master Jun 29, 2021
@kaiyan-sheng kaiyan-sheng deleted the aws_with_input_groups branch June 29, 2021 23:28
eyalkraft pushed a commit to build-security/integrations that referenced this pull request Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants