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

Multiple attributes per node #964

Merged
merged 4 commits into from
Jun 20, 2024
Merged

Multiple attributes per node #964

merged 4 commits into from
Jun 20, 2024

Conversation

elijahbenizzy
Copy link
Collaborator

No description provided.

@elijahbenizzy elijahbenizzy force-pushed the multiple-attributes branch 3 times, most recently from de47c6b to a09df1e Compare June 17, 2024 23:20
@elijahbenizzy
Copy link
Collaborator Author

image

…me task

Before we were joining by task name which is incorrect. We can have multiple. This fixes it.
This allows us to display multiple logged attributes. The names are just
title-case versions of the keys.
We can't control the environment, but it's printing out the wrong one
for mini-mode, which confuses people. If it prints it out for postgres mode in local env it
should redirect through a prompt in the UI.
@elijahbenizzy elijahbenizzy changed the title Multiple attributes per node (WIP) Multiple attributes per node Jun 18, 2024
@elijahbenizzy
Copy link
Collaborator Author

elijahbenizzy commented Jun 18, 2024

Didn't add tests to the sdk cause this wasn't explicitly tested before, this is a very small change. We have tests on the server side of duplication of task updates.

@elijahbenizzy elijahbenizzy marked this pull request as ready for review June 18, 2024 00:12
@elijahbenizzy elijahbenizzy requested a review from skrawcz June 18, 2024 00:12
@@ -1,2 +1,2 @@
HAMILTON_API_URL = "http://localhost:8241"
HAMILTON_UI_URL = "http://localhost:8242"
HAMILTON_UI_URL = "http://localhost:8241" # optimizing for mini-mode
Copy link
Collaborator

Choose a reason for hiding this comment

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

err -- docs need to be updated then for use with docker then right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

or there needs to be a way to know which URL to use that the SDK can figure out

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See commit notes, this was the quickest change but yeah we can update docs

Copy link
Collaborator

@zilto zilto Jun 18, 2024

Choose a reason for hiding this comment

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

side note: 127.0.0.1 should be preferred to localhost ref.

Also, for mini-mode, the entrypoint 127.0.0.1 should be preferred to 0.0.0.0. But this would provide minimal value if requires a bigger refactoring because Docker-mode would require 0.0.0.0 most likely

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm I'm generally against using a hardcoded value for the purpose of small optimizations, although there are other reasons... Good video.

Copy link
Collaborator

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

yes, but you need to add to docs on how to change the URL to point to wherever the UI lives.

@elijahbenizzy elijahbenizzy merged commit a91b940 into main Jun 20, 2024
27 checks passed
@elijahbenizzy elijahbenizzy deleted the multiple-attributes branch June 20, 2024 04:44
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.

3 participants