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

isisdataeval Updates, fixes and additional documentation #5163

Merged
merged 11 commits into from
May 4, 2023

Conversation

KrisBecker
Copy link
Contributor

Apologies I missed the merge of #5111 with a few, but perhaps helpful, updates.

isisdata_mockup.py - Added —tojson and —hasher parameters to improve utility

make_isisdata_mockup.sh - Fixed path to test data and added new parameters to the isisdata_mockup.py introduced in this PR

README.md - improved/clarified/updated documentation

Description

Related Issue

#5111

How Has This Been Validated?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Documentation change (update to the documentation; no code change)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Infrastructure change (changes to things like CI or the build system that do not impact users)

Checklist:

  • I have read and agree to abide by the Code of Conduct
  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • I have added myself to the .zenodo.json document.
  • I have added any user impacting changes to the CHANGELOG.md document.

Licensing

This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words:

This work is free and unencumbered software released into the public domain. In jurisdictions that recognize copyright laws, the author or authors of this software dedicate any and all copyright interest in the software to the public domain.

  • I dedicate any and all copyright interest in this software to the public domain. I make this dedication for the benefit of the public at large and to the detriment of my heirs and successors. I intend this dedication to be an overt act of relinquishment in perpetuity of all present and future rights to this software under copyright law.

isisdata_mockup.py - Added —tojson and —hasher parameters to improve utility

make_isisdata_mockup.sh - Fixed path to test data and added new parameters to the isisdata_mockup.py introduced in this PR

README.md - improved/clarified/updated documentation
The CHANGELOG.md was incorrectly merged as the entry for this app was added in the wrong section, which seem wierd. This commit corrects and updates the log.
@KrisBecker
Copy link
Contributor Author

Question: Is it recommended/possible to install isisdata_mockup.py in a bin-type directory or someplace where its easier to access/use?

Copy link
Collaborator

@Kelvinrr Kelvinrr left a comment

Choose a reason for hiding this comment

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

some small comments.

Partly also my bad for being a little hasty with wanting to merge that PR.

isis/src/system/apps/isisdataeval/tools/README.md Outdated Show resolved Hide resolved
isis/src/system/apps/isisdataeval/tools/README.md Outdated Show resolved Hide resolved
@KrisBecker
Copy link
Contributor Author

Any suggestions on where/how to install isisdata_mockup.py? This should be conveniently available to ISIS users if you decide to use this script as mechanism to validate ISISDATA installations.

@Kelvinrr
Copy link
Collaborator

Kelvinrr commented Apr 3, 2023

@KrisBecker at the end of the CMakeLists.txt, you can add a line to install it to $INSTALL_PREFIX/bin/ with the .py stripped so we can run it as a command. That's how we've usually done these utility scripts we want common accesss to.

@KrisBecker
Copy link
Contributor Author

Ok, so should I scrub all the docs to refer to the renamed extension-less version of the script? At least in the READMEs and the tests scripts?

isisdata_mockup.py - Install this script in $ISISROOT/bin as isisdata_mockup (no extension)

make_isisdata_mockup.sh - Renamed isisdata_mockup.py to isisdata_mockup

CMakeLists.txt - Add install command for isisdata_mockup
@KrisBecker
Copy link
Contributor Author

Commits install isisdata_mockup in $ISISROOT/bin (its not in build/bin). Had to finesse the permissions.

I also changed the test data mocking script to call this version.

This does create an issue for most of the documentation. It may be easier to just rename the script without the extension and change all the documentation appropriately otherwise I am concerned trying to retain the extension will lead to confusion.

@Kelvinrr
Copy link
Collaborator

Kelvinrr commented Apr 4, 2023

That would be helpful, yes.

To ease use and minimize confusion, renamed isisdata_mockup.py to isisdata_mockup (w/o the .py extension)

Updated documentation and install to reflect the change
@KrisBecker
Copy link
Contributor Author

Ok, renamed isisdata_mockup and modified docs/scripts to reflect the change.

This PR should be good to go now.

@KrisBecker
Copy link
Contributor Author

Please let me know if there is anything else I need to do in order to get this PR merged.

Thank you.

Kelvinrr
Kelvinrr previously approved these changes Apr 29, 2023
@Kelvinrr
Copy link
Collaborator

Once merge conflicts are resolved I'll merge this.

This push resolves a merge conflict in the CHANGELOG.md

Added a entry to CHANGELOG.md to describe the PR changes.
@KrisBecker
Copy link
Contributor Author

Merge conflict resolved in CHANGELOG.md and added an entry with these updates.

@Kelvinrr
Copy link
Collaborator

Kelvinrr commented May 2, 2023

There is a random error after the merge conflicts were fixed in the test.

The following tests FAILED:
	1528 - DemCube.FunctionalTestSpiceinitWebAndShapeModel (Failed)

Might be a fluke so I am rerunning Jenkins for now.

@Kelvinrr
Copy link
Collaborator

Kelvinrr commented May 4, 2023

the error is still there, it did show up after the last commit. But it is a Spiceinit error trying to hit the web server. Might have been down at the time the test ran coincidentally. I'll run one more time, if it still fails on that test it will probably have to be debugged.

Copy link
Collaborator

@Kelvinrr Kelvinrr left a comment

Choose a reason for hiding this comment

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

aaaaaay it passed.

@Kelvinrr Kelvinrr merged commit 05a90e9 into DOI-USGS:dev May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants