-
Notifications
You must be signed in to change notification settings - Fork 3
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
Refactor harvest to operate with new multi-tenant, serverless OpenSearch architecture #146
Conversation
@alexdunnjpl @jordanpadams @nutjob4life @tloubrieu-jpl Schema has been implemented and examples updated. Would like to review because older "legacy" harvest config files cannot be used without minor changes. Should not be a problem because URL is not necessary for non-testing harvest configs. Can review be done during today's breakout? It would be good to keep going with this but having to change tag names or something after this will become harder and harder. |
Current state of schema and tests are:
Left the name broken in each just to show that xmllint is working as expected. |
@al-niessner @tloubrieu-jpl was this review completed at the breakout yesterday? |
|
These tests required files that are no longer around - as in /tmp. Also removed the dead code from remaining tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@al-niessner a few comments. thoughts?
<xs:all> | ||
<xs:element minOccurs="0" | ||
name="autogenFields" type="autogen_fields_type"/> | ||
<xs:element name="do" type="do_type"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@al-niessner Do we really need this? I would prefer to minimize the number of backwards incompatible changes if we can?
But if you would really prefer we encapsulte this, can we change this to data_config
or load_config
or just data
or load
?
Changed some the items and suggest do -> load or maybe ingest. Changed direct_url to server_url since that is really what it is and you like self documenting names. Now cognito_client_id for same reason. Take a gander again and I will update code again. I need to run the local test to make sure I have not broken anything but I do not think so. |
At this point (52aec27) can use harvest to load bundle from #143 with what seem to be the same results:
Next is to start looking at modifying registry-common to have better registry connection interface than a public bean with no setter/getter that will allow for polymorphism to help when there are different ways to contact different services like coginito or direct - the cause of the original issue. |
@al-niessner note we needed to update the The pre-commit and info about Detect Secrets are being added to Harvest README here: #150 But that pretty much links here: https://github.com/NASA-PDS/nasa-pds.github.io/wiki/Git-and-Github-Guide#detect-secrets |
🗒️ Summary
Brief summary of changes if not sufficiently described by commit messages.
⚙️ Test Data and/or Report
One of the following should be included here:
♻️ Related Issues
Closes #118