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

Update settings.php for data_pipelines url #271

Open
wants to merge 1 commit into
base: 6.x
Choose a base branch
from

Conversation

vincent-gao
Copy link
Contributor

@vincent-gao vincent-gao commented Aug 6, 2024

Checklist

  • Affected Issues have been mentioned in the Closing issues section
  • Documentation has been written/updated.
  • Changelog entry has been written

Explain the details for making this change. What existing problem does the pull request solve?

Changelog Entry

This PR relates to

the sdp_elasticsearch_v1 destination will be available for other projects.
sdp_elasticsearch_v1 supports
- dpc-sdp/tide_search#85
- and (maybe) more new features that only available on SDP

We don't want to remove the sdp_elasticsearch destination for now

@vincent-gao vincent-gao self-assigned this Aug 6, 2024
@vincent-gao vincent-gao added the NEEDS REVIEW Good for newcomers label Aug 6, 2024
$elasticsearch_url = (getenv('SEARCH_HASH') && getenv('SEARCH_URL')) ? sprintf('http://%s.%s', getenv('SEARCH_HASH'), getenv('SEARCH_URL')) : "http://elasticsearch:9200";
$data_pipeline_configs = [
'sdp_elasticsearch',
'sdp_elasticsearch_v1',
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant to ask in the other PR.... what's v1 for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

v1 represents version 1 😜
sdp_elasticsearch_v1 indicates it uses the Destination plugin that defined by tide_data_pipeline module.

Copy link
Contributor

Choose a reason for hiding this comment

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

We've had to override the existing plugin to get new functionality?

Copy link
Contributor Author

@vincent-gao vincent-gao Aug 7, 2024

Choose a reason for hiding this comment

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

We've had to override the existing plugin to get new functionality?

it is the content thing, we can't override it, we have to select it via datapipeline content e.g. https://develop.content.vic.gov.au/admin/content/datasets/manage/3/edit?destination=/admin/content/datasets
CleanShot 2024-08-07 at 14 43 02

The plan of mine is for these 2 types of Destinations to coexist for now. If it is a new data pipeline or needs to be switched to v1, we only need to select v1 in the datapipeline content

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have enough context to approve this PR. @anthony-malkoun are you happy with Vincent's justification?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NEEDS REVIEW Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants