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.

Sign up for GitHub

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

Jump to bottom

Show wk user in vx workflow list #7794

Merged
merged 32 commits into from
Jun 3, 2024
Merged

Show wk user in vx workflow list #7794

merged 32 commits into from
Jun 3, 2024

Conversation

knollengewaechs
Copy link
Contributor

@knollengewaechs knollengewaechs commented May 8, 2024 •
edited
Loading

Steps to test:

  • Install voxelytics and follow instructions in worker readme (set directory and enable jobs)
  • start a job such as computing a mesh file
  • go to voxelytics overview and see your current wk user name in table
  • try out filter of user column

TODOs:

  • host user only as backup for wk user
  • view workflows of multiple users -> change in pgadmin
  • implement user filter

Issues:

(Please delete unneeded items, merge only when none are left open)

@knollengewaechs knollengewaechs self-assigned this May 12, 2024
knollengewaechs
knollengewaechs commented May 16, 2024
View reviewed changes
conf/application.conf Outdated
Comment on lines 153 to 154
jobsEnabled = false
voxelyticsEnabled = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as always ;) dont forget to remove!

knollengewaechs
knollengewaechs commented May 21, 2024
View reviewed changes
app/models/voxelytics/VoxelyticsService.scala
@@ -77,14 +77,16 @@ object ChunkCounts {

case class WorkflowListingRunEntry(id: ObjectId,
name: String,
username: String,
hostusername: String,
hostname: String,
voxelyticsVersion: String,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

upon first sight it seems to me that the naming convention is mixed here (snake case vs. camel case vs. all small letters). Would this be an opportunity to unify the properties or is it on purpose?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, feel free to camelCase this stuff!

Also, hostusername can probably be thrown out, as the frontend does not display it anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright, I'll do that! actually the user name on the host is displayed if the wk user couldnt be found, so it is still used. is it okay if I keep it in?

Copy link
Member

Choose a reason for hiding this comment

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

Ah now I see the left join. Ok fair, if we want to support the rare case of a deleted user. However, in this case the type in scala needs to be Option[String]. And please test locally once that the query still behaves correctly if the user is missing/deleted.

But yes, then let’s keep the hostusername in as a fallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

local testing after deleting the user from the database went well :)
image

fm3
fm3 reviewed May 22, 2024
View reviewed changes
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Backend LGTM. I’d suggest to remove hostusername from the case class and query, as it is no longer displayed.

@knollengewaechs knollengewaechs marked this pull request as ready for review May 22, 2024 20:39
@knollengewaechs
Copy link
Contributor Author

knollengewaechs commented May 29, 2024 •
edited
Loading

@fm3 I renamed the scala class members as discussed in the other thread. This lead to a few changes, can you have another look? I tested it locally and everything seems to work like before.

philippotto
philippotto reviewed May 30, 2024
View reviewed changes
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

front-end looks good to me 🎉 only left one remark about proper sorting. will leave the final approval to @fm3.

frontend/javascripts/admin/voxelytics/workflow_list_view.tsx Outdated Show resolved Hide resolved
fm3
fm3 approved these changes May 30, 2024
View reviewed changes
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Backend LGTM and re-testing looked good :) Feel free to merge after addressing Philipp’s latest comment

@knollengewaechs knollengewaechs enabled auto-merge (squash) June 3, 2024 14:55
@knollengewaechs knollengewaechs merged commit 21e08f8 into master Jun 3, 2024
2 checks passed
@knollengewaechs knollengewaechs deleted the show-wk-user-in-vx-workflow-list branch June 3, 2024 15:12
knollengewaechs added a commit that referenced this pull request Jun 3, 2024
@knollengewaechs
Revert "Show wk user in vx workflow list (#7794)"
4dede82 
This reverts commit 21e08f8.
@knollengewaechs knollengewaechs mentioned this pull request Jun 3, 2024
Merged
7 tasks
knollengewaechs added a commit that referenced this pull request Jun 3, 2024
@knollengewaechs
Revert wk name in vx workflow list (#7853)
814919a 
* Revert "Fix camel case so that host is shown (#7852)"

This reverts commit de7b9d5.

* Revert "Show wk user in vx workflow list (#7794)"

This reverts commit 21e08f8.
MichaelBuessemeyer
MichaelBuessemeyer reviewed Jun 4, 2024
View reviewed changes
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

I only found one small thing that might have an effect on the correctness of these changes, but I am not sure 🥴

frontend/javascripts/admin/voxelytics/workflow_list_view.tsx
filterSearch: true,
},
{
title: "Hostname",
title: "Host",
dataIndex: "hostname",
Copy link
Contributor

Choose a reason for hiding this comment

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

The prop hostname was renamed to hostName right? But not here? is this maybe something?

@knollengewaechs knollengewaechs mentioned this pull request Jun 5, 2024
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philippotto philippotto philippotto left review comments

@MichaelBuessemeyer MichaelBuessemeyer MichaelBuessemeyer left review comments

@fm3 fm3 fm3 approved these changes

Assignees

@knollengewaechs knollengewaechs

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show WK user name next to owner in voxelytics workflows list
4 participants
@knollengewaechs @fm3 @philippotto @MichaelBuessemeyer