-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Update UI to use new allocated ports fields #8631
Conversation
Ember Asset Size actionAs of cce6215 Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
I'm recusing myself since we paired on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can’t comment on the Go but the Ember looks good! I pointed out a couple of stale page object properties but nothing serious.
const dynamicPorts = taskResources.resources.Networks[0].DynamicPorts; | ||
const addresses = reservedPorts.concat(dynamicPorts); | ||
|
||
assert.equal(Task.addresses.length, addresses.length, 'All addresses are listed'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be addressed (😬) separately if there’s a time crunch but I think with the removal of these tests, the Task.addresses
and .hasAddresses
page objects can be deleted, I couldn’t find anywhere else they were used.
{{/each}} | ||
{{/let}} | ||
</ul> | ||
</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty unimportant but this means the corresponding tasks.ports
property in tests/pages/allocations/detail
can be deleted. I feel like there are probably several existing orphaned page objects properties though as we don’t have a good way to detect things we aren’t using anymore.
hieeeeee in #8634 I have a serialiser abstraction that is (minorly) useful in this PR, could be nice to have this merged so I can update that before merging, let me know if there’s something I can contribute to get this merged? I could, for instance, make the deletions I suggested above. |
Hey @backspace could you take one more look to make sure I removed everything you mentioned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great, yes, thanks for removing those!! 🥳
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
In Nomad 0.12 a new field in the
Allocation.AllocatedResources.Shared
object was added which lists all allocated ports for the allocation and the associated host address. This PR updates the UI to use these new fields and removes references to task networks/ports since they are deprecated and will be removed.