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

Add "user-project" option to accept a user-defined project name #482

Merged
merged 3 commits into from
Mar 29, 2024

Conversation

HughNhan
Copy link
Contributor

Synopsis:

Add another flavor of setting run project. Currently crucible-rickshaw is the default project. There is also the "unique-project" option which autogenerates a project name with UUID. This new "user-project" option allows the user to specify a user-defined project name.

Unit test:

  1. Default project name crucible-rickshaw works correctly when no unique-project or user-project option.
  2. unique-project option works correctly
  3. user-project works correctly

@HughNhan HughNhan requested review from k-rister and atheurer March 27, 2024 01:38
Copy link
Contributor

@k-rister k-rister left a comment

Choose a reason for hiding this comment

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

So, I think I have to start by asking why is this needed? I would assume it is because the user defined project name already exists and has been endowed with certain privileges/capabilities/etc. that the user needs. If that is the case, then I think you might want to add an additional piece of logic to skip the namespace deletion to avoid tearing down that user defined project. If this is not the case then I don't really understand why we need this.

Next, I think I would expect a different order of precedence. If the user goes to the trouble of specifing a user-project then it should probably have the highest priority instead of unique-project.

@HughNhan
Copy link
Contributor Author

HughNhan commented Mar 27, 2024

So, I think I have to start by asking why is this needed? I would assume it is because the user defined project name already exists and has been endowed with certain privileges/capabilities/etc. that the user needs. If that is the case, then I think you might want to add an additional piece of logic to skip the namespace deletion to avoid tearing down that user defined project. If this is not the case then I don't really understand why we need this.

The new "user-project" option supports multiple users sharing a testbed more conveniently. By having a user-defined namespace each can manually manage their run more conveniently. During debug, I abort/CTK-C a run quite often. I can identify my objects more easily with my namespace.

Next, I think I would expect a different order of precedence. If the user goes to the trouble of specifing a user-project then it should probably have the highest priority instead of unique-project.

The precedence is for code robustness. Perhap due to a typo/mistake the run CLI has both options. In this situation, the new, "user-project" should be overridden because the incumbent "unique-project" has more usage history.

@HughNhan HughNhan requested a review from k-rister March 28, 2024 13:31
@k-rister
Copy link
Contributor

So, I think I have to start by asking why is this needed? I would assume it is because the user defined project name already exists and has been endowed with certain privileges/capabilities/etc. that the user needs. If that is the case, then I think you might want to add an additional piece of logic to skip the namespace deletion to avoid tearing down that user defined project. If this is not the case then I don't really understand why we need this.

The new "user-project" option supports multiple users sharing a testbed more conveniently. By having a user-defined namespace each can manually manage their run more conveniently. During debug, I abort/CTK-C a run quite often. I can identify my objects more easily with my namespace.

Ok, so deletion isn't an issue.

Next, I think I would expect a different order of precedence. If the user goes to the trouble of specifing a user-project then it should probably have the highest priority instead of unique-project.

The precedence is for code robustness. Perhap due to a typo/mistake the run CLI has both options. In this situation, the new, "user-project" should be overridden because the incumbent "unique-project" has more usage history.

I'm not sure I agree with this. Our CLI leaves a lot to be desired and is one the the reasons why we are switching to JSON so that historical aspect/conflict doesn't really bother me. I'm curious what @atheurer thinks?

@k-rister
Copy link
Contributor

It might make more sense, and avoid the precendence conflict, to change this so that the only parameter is unique-project. If the value is 1 then it would autogenerate a project name as it does today, if it is anything else it could use that value for the project name.

@HughNhan
Copy link
Contributor Author

It might make more sense, and avoid the precendence conflict, to change this so that the only parameter is unique-project. If the value is 1 then it would autogenerate a project name as it does today, if it is anything else it could use that value for the project name.

I will make that change and resubmit.
Thanks Karl!

Copy link
Contributor

@k-rister k-rister left a comment

Choose a reason for hiding this comment

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

Don't you need to tweak this as well:

    if [ "${unique_project}" == "1" ]; then
        project_name+="--${run_id}-${endpoint_label}"
    fi

@HughNhan
Copy link
Contributor Author

Don't you need to tweak this as well:

    if [ "${unique_project}" == "1" ]; then
        project_name+="--${run_id}-${endpoint_label}"
    fi

The submit code is currently like this:

if [ "${unique_project}" == "1" ]; then
    project_name+="--${run_id}-${endpoint_label}"
elif [ ! -z "${unique_project}" ]  &&  [ "${unique_project}" != "0" ]; then
    project_name="${unique_project}"
fi

}

@k-rister
Copy link
Contributor

Hmm, so it is. For some reason when I was looking at the changes yesterday I didn't see that part, but I do now...weird.

@k-rister k-rister merged commit 62df957 into master Mar 29, 2024
138 checks passed
@k-rister k-rister deleted the user_project branch March 29, 2024 13:15
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