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.

Sign up for GitHub

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

Jump to bottom

feat:An ability to override Kibana image URL in DA #345

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

feat:An ability to override Kibana image URL in DA #345

wants to merge 21 commits into from
+320 −316

Conversation

aatreyee2506
Copy link

@aatreyee2506 aatreyee2506 commented Nov 19, 2024 •
edited by Aayush-Abhyarthi
Loading

Description

Added a variable "kibana_image_reference" to give the users an option to override the kibana image.
Git issue

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

This PR adds variable "kibana_image_reference" to give the users an option either to go with the default image or give their own image as an input.

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@aatreyee2506
Copy link
Author

/run pipeline

@aatreyee2506
Copy link
Author

aatreyee2506 commented Nov 19, 2024 •
edited
Loading

cc: @SarikaSinha @maheshwarishikha

ocofaigh
ocofaigh requested changes Nov 19, 2024
View reviewed changes
Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

See comments. Also please update the variable description for enable_kibana_dashboard and make sure to reference the new kibana_image_reference input.

Also any time you add a new input variable to a DA solution, you need to also add it to the ibm_catalog.json file. The variables ion that file are sorted in an order that keeps related variables group together. So ensure to add the new variable with the other kibana related variable inputs

solutions/standard/variables.tf Outdated Show resolved Hide resolved
solutions/standard/variables.tf Outdated Show resolved Hide resolved
solutions/standard/main.tf Outdated Show resolved Hide resolved
solutions/standard/variables.tf Show resolved Hide resolved
changed the description for kibana_image_reference ,enable_kibana_das…
f7020fd 
…hboard and elasticsearch_full_version,changed empty string to null added variable in ibm_catalog.json
@aatreyee2506
Copy link
Author

/run pipeline

ocofaigh
ocofaigh requested changes Nov 20, 2024
View reviewed changes
Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

see latest comments

ibm_catalog.json Show resolved Hide resolved
solutions/standard/variables.tf Outdated Show resolved Hide resolved
solutions/standard/variables.tf Outdated Show resolved Hide resolved
solutions/standard/variables.tf Outdated Show resolved Hide resolved
@ocofaigh
Copy link
Member

@aatreyee2506 before running the pipeline, ensure your branch is up to date with latest code in main branch - otherwise the pipeline needs to be re-run again after rebase (which has an associated cost to it since we provision actual resources in our account)

@aatreyee2506
Copy link
Author

/run pipeline

@Ak-sky Ak-sky mentioned this pull request Nov 21, 2024
Closed
@aatreyee2506
Copy link
Author

/run pipeline

@ocofaigh
Copy link
Member

ocofaigh commented Dec 4, 2024

@aatreyee2506 @Ak-sky Why was the PR closed?

@Ak-sky
Copy link
Member

Ak-sky commented Dec 4, 2024 •
edited
Loading

@aatreyee2506 @Ak-sky Why was the PR closed?

@ocofaigh, it was by mistake, reopening this.

@Ak-sky Ak-sky reopened this Dec 4, 2024
shemau
shemau requested changes Dec 5, 2024
View reviewed changes
solutions/standard/variables.tf Outdated Show resolved Hide resolved
@Ak-sky Ak-sky requested review from Ak-sky and shemau December 6, 2024 13:11
@aatreyee2506
Copy link
Author

/run pipeline

ocofaigh
ocofaigh requested changes Dec 11, 2024
View reviewed changes
Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

see comments

This was referenced Dec 13, 2024
Draft
Open
@ocofaigh
Copy link
Member

@aatreyee2506 @Ak-sky any update on this?

@aatreyee2506
Copy link
Author

Hi @ocofaigh ,working on this now.

@aatreyee2506
Copy link
Author

/run pipeline

ocofaigh
ocofaigh requested changes Dec 18, 2024
View reviewed changes
Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

see comments

solutions/standard/variables.tf Outdated Show resolved Hide resolved
solutions/standard/variables.tf Outdated Show resolved Hide resolved
solutions/standard/variables.tf Outdated Show resolved Hide resolved
solutions/standard/variables.tf Outdated Show resolved Hide resolved
solutions/standard/main.tf Outdated Show resolved Hide resolved
@aatreyee2506
Copy link
Author

/run pipeline

@Ak-sky
Copy link
Member

Ak-sky commented Dec 20, 2024

/run pipeline

@aatreyee2506
Copy link
Author

/run pipeline

@imprateeksh
Copy link
Member

@aatreyee2506 - Please do not commit common-dev-assets submodule updates in the PR. This could cause conflicts. We do have renovate automation that handles updating the submodule.

imprateeksh
imprateeksh reviewed Dec 26, 2024
View reviewed changes
Copy link
Member

@imprateeksh imprateeksh left a comment

Choose a reason for hiding this comment

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

@aatreyee2506 Provided some comments, please check.

ibm_catalog.json Outdated Show resolved Hide resolved
solutions/standard/variables.tf Outdated Show resolved Hide resolved
solutions/standard/variables.tf Outdated Show resolved Hide resolved
@aatreyee2506
Copy link
Author

/run pipeline

imprateeksh
imprateeksh reviewed Dec 27, 2024
View reviewed changes
Copy link
Member

@imprateeksh imprateeksh left a comment

Choose a reason for hiding this comment

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

Left some more comments.

solutions/standard/variables.tf Show resolved Hide resolved
solutions/standard/variables.tf Show resolved Hide resolved
solutions/standard/main.tf Show resolved Hide resolved
solutions/standard/variables.tf Outdated Show resolved Hide resolved
@imprateeksh
Copy link
Member

/run pipeline

@aatreyee2506
Copy link
Author

/run pipeline

@aatreyee2506
Copy link
Author

/run pipeline

imprateeksh
imprateeksh approved these changes Jan 2, 2025
View reviewed changes
Copy link
Member

@imprateeksh imprateeksh left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ocofaigh ocofaigh ocofaigh requested changes

@imprateeksh imprateeksh imprateeksh approved these changes

@daniel-butler-irl daniel-butler-irl Awaiting requested review from daniel-butler-irl daniel-butler-irl is a code owner

@Ak-sky Ak-sky Awaiting requested review from Ak-sky

@shemau shemau Awaiting requested review from shemau shemau is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aatreyee2506 @ocofaigh @Ak-sky @imprateeksh @shemau