-
Notifications
You must be signed in to change notification settings - Fork 15
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
Improve DataFed setup configuration options #920
Open
JoshuaSBrown
wants to merge
5
commits into
devel
Choose a base branch
from
JoshuaSBrown-feature-improve-python-client-setup-config
base: devel
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Improve DataFed setup configuration options #920
JoshuaSBrown
wants to merge
5
commits into
devel
from
JoshuaSBrown-feature-improve-python-client-setup-config
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JoshuaSBrown
added
Type: New Feature
New or enhanced feature
Component: Python API
Relates to Python API
Component: CLI
Relates to command-line interface
Type: Refactor
Imlplementation change, same functionality
Priority: Medium
Above average priority
Type: Test
Related to unit or integration testing
Status: Discuss
Needs discussion to determine proper disposition
labels
Nov 7, 2023
JoshuaSBrown
changed the title
Update python version
Improve DataFed setup configuration options
Nov 7, 2023
This looks good. A few comments.
|
For parts 1-3, I'm concerned about the behavior deviating from what it is currently doing, if I change the behavior of the setup command then it becomes a breaking change. I realize keeping "datafed setup" command the same is not an ideal solution, but I'm concerned with breaking backward compatibility. Let me discuss it a bit with Dale.
best,
PS Thanks for the feedback.
Joshua
From: Joshua C Agar ***@***.***>
Sent: Tuesday, November 7, 2023 10:58 AM
To: ORNL/DataFed ***@***.***>
Cc: Brown, Joshua ***@***.***>; Assign ***@***.***>
Subject: [EXTERNAL] Re: [ORNL/DataFed] Improve DataFed setup configuration options (PR #920)
This looks good. A few comments.
1. I think you should provide some definitions of what each of these categories are, and what they are responsible for doing. I am unclear the true demarkation from user and server credentials.
2. What would be the default option on datafed setup? I feel it should delete everything? or at least prompt the user if they want to delete their configuration.
3. If it is not the default, I would recommend error handling that alerts the user that the configuration files are not correct.
-
Reply to this email directly, view it on GitHub<https://urldefense.us/v2/url?u=https-3A__github.com_ORNL_DataFed_pull_920-23issuecomment-2D1799019939&d=DwMCaQ&c=v4IIwRuZAmwupIjowmMWUmLasxPEgYsgNI-O7C4ViYc&r=EGSrZDU--J4wp05SUEemL_ukuVkVGS0SEKwYtlhw5Rs&m=5xamwhuIpIJqKjoW1UVKO9Kj3OJLYsBIPXPigAHwDAqXgv1i769V75_fkvrdwdW-&s=5G3g8Yw4iC-aTDM9Vp4HZqCgraiU6V4Ntkp2HP4Ghmk&e=>, or unsubscribe<https://urldefense.us/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ABDSZHEIP6AYAHA7KQZVIDLYDJK7NAVCNFSM6AAAAAA7BK45K6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJZGAYTSOJTHE&d=DwMCaQ&c=v4IIwRuZAmwupIjowmMWUmLasxPEgYsgNI-O7C4ViYc&r=EGSrZDU--J4wp05SUEemL_ukuVkVGS0SEKwYtlhw5Rs&m=5xamwhuIpIJqKjoW1UVKO9Kj3OJLYsBIPXPigAHwDAqXgv1i769V75_fkvrdwdW-&s=LlSQX_8BWIZ5FVOG_OxXDw8E_z9I3jvCIC-ChTg1rC4&e=>.
You are receiving this because you were assigned.Message ID: ***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Component: CLI
Relates to command-line interface
Component: Python API
Relates to Python API
Priority: Medium
Above average priority
Status: Discuss
Needs discussion to determine proper disposition
Type: New Feature
New or enhanced feature
Type: Refactor
Imlplementation change, same functionality
Type: Test
Related to unit or integration testing
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Users have reported several problems with the datafed setup process when working with the python client. The setup step currently, is responsible for creating files in the users home directory in an invisible .datafed directory. These files include, the users public and private keys .pub and .priv as well as the core servers public key which is downloaded from the web server with an https request. Lastly, in the .datafed folder is a .ini file which contains the configuration the client will use.
Two problems will be addressed in this PR.
Implementation
NOTE: we are not proposing changing the default behavior of the command:
This command will remain the same, this command looks to see if public and private keys exist and if a configuration file exists. It will use the existing files if they do exist and only attempts do something if a file is missing so:
Good defaults and wiping configuration cleanly
Possible CLI implementations should not change the existing CLI - so the changes should be backwards compatible.
Feedback is wanted on what command line flags make the most sense?
For clearing or resetting the configuration to defaults and recreating the credentials and keys, possible flag names could be:
For just resetting the credentials (Meaning the public keys for the user and the core server as well as the users private key) but without changing the configuration (the .ini settings).
For just resetting the user credentials even if the public and private key for the user already exist.
For resetting the public key of the core server. This would require redownloading the core server public key from the datafed web server over https. In the future, if the metadata server because federated and there are multiple metadata servers it will be important to be able to point to the correct metadata server to ensure the right public key is used (That assumes each metadata server has a different public key.
For providing better transparency
To show the actual configuration that is being used when the datafed CLI is used it should be clear when env variables are used and or variables from the command line or from a configuration file. It should also be clear if a default is being used.
Show contents and path to the configuration file that is being used.
Expected Scope of Changes
This is expected to effect the following functions, which will likely need changes:
In addition, several functions will need to be added in the CommandLib.py file to allow for the capabilities indicated previously.
Tasks
Work time
Expected time: 16 hours