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

New feature to allow harvest to set archive status #161

Merged
merged 2 commits into from
Aug 9, 2024
Merged

Conversation

al-niessner
Copy link
Contributor

@al-niessner al-niessner commented Aug 2, 2024

🗒️ Summary

Allow harvest to define the archive status when it harvests the data.

⚙️ Test Data and/or Report

Validated by setting to certified and looked in DB. Also tried with fred and it failed giving a list of valid choices.

♻️ Related Issues

Closes #135
Requires NASA-PDS/registry-common#56

Al Niessner added 2 commits August 2, 2024 12:17
registry-common changed to go from hard coded value to a parameter. Changes in this commit support the new parameter all the way to the CLI.
Added the option to set the archive tested. Tested with certified and it worked. Also tested with fred and it failed as not legal.
@al-niessner al-niessner self-assigned this Aug 2, 2024
@al-niessner al-niessner requested a review from a team as a code owner August 2, 2024 20:56
@jordanpadams jordanpadams changed the title Issue 135: update archive status handling New feature to allow harvest to set archive status Aug 7, 2024
@jordanpadams
Copy link
Member

@al-niessner is there any kind of integration tests we could add to the our registry test suite to verify the setting of archive status upon loading of the data?

@al-niessner
Copy link
Contributor Author

@jordanpadams

We could add a trivial test that show that the default variable is updated. Not sure that does much but could offer a very modest amount of regression. Otherwise back to needing a running opensearch, secrets, test data, expectations that change with the data etc.

@jordanpadams
Copy link
Member

@al-niessner thanks. I don’t think a test like that is necessary worth it. I will try to test this over the weekend.

@tloubrieu-jpl
Copy link
Member

@al-niessner @jordanpadams, I suspect we will encounter the same issue that we are having on NASA-PDS/registry-mgr#82 (comment)

But for now I am having a different issue:

% ./bin/registry-manager set-archive-status \
    -auth /Users/loubrieu/Documents/pds/registry/es-auth.txt \
    -es file:/Users/loubrieu/Documents/pds/registry/mcp_dev.xml \
    -packageId 8d12a9ba-2ba0-4d80-8ce9-65da271ecf89 \
    -status archived

[ERROR] Missing required parameter '-lidvid' or '-packageId

@al-niessner is that something you can reproduce ?

Thanks

@al-niessner
Copy link
Contributor Author

@tloubrieu-jpl

This is about harvest not registry-mgr. Not sure if something is wrong in your environment or just putting messages in different PRs.

@tloubrieu-jpl
Copy link
Member

Sorry @al-niessner, I was confused about what the PR was for. I will test that again, more appropriately.

@tloubrieu-jpl
Copy link
Member

I was finally able to validate that new development by manual test on MCP Dev account.

Copy link
Member

@tloubrieu-jpl tloubrieu-jpl left a comment

Choose a reason for hiding this comment

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

That works perfectly on MCP Dev.

Thanks @al-niessner

@tloubrieu-jpl tloubrieu-jpl merged commit 080142c into main Aug 9, 2024
2 checks passed
@tloubrieu-jpl tloubrieu-jpl deleted the issue_135 branch August 9, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

As a user, I want the default archive_status for loaded products to be configurable
3 participants