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

refact: update models for relationship #17315

Merged
merged 3 commits into from
May 31, 2023

Conversation

ChaiWithJai
Copy link
Contributor

Resolves #17293

This PR adds a relationship between the Node and NodePool models. In addition, The Job model now has an @attr('string') property called nodePool.

The nodePool property on the Node model is used to store the name of the NodePool that the job is associated with. The nodes property on the NodePool model is used to store a list of the Job models that are associated with the NodePool.

This change was made to allow users to easily associate jobs with node pools. For example, a user could create a job and specify the name of the node pool that the job should be run on.

Testing
The changes in this PR have been tested using the following tests:

ui/tests/unit/models/node-pool-test.js

The tests have been written to ensure that the Node and NodePool models can be associated with each other correctly.

Impact
This change will have a minor impact on the codebase. The Node and NodePool models will now have a relationship with each other.

Next Steps
The next steps for this change are to seed the Mirage environments such that node pools are reflected. Once that is ready, we'll be ready to visualize the changes in the UI against both a Mirage scenario so that design can sign off and against the real API changes to smoke test the changes.

@github-actions
Copy link

github-actions bot commented May 25, 2023

Ember Test Audit comparison

17292/node-pool-ember-data 1619994 change
passes 1476 1477 +1
failures 0 0 0
flaky 0 0 0
duration 11m 26s 786ms 10m 46s 837ms -39s 949ms

Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Looking good! Just left some minor comments and suggestions

ui/app/models/job.js Outdated Show resolved Hide resolved
module('Unit | Model | NodePool', function (hooks) {
setupTest(hooks);

test('a node pool can be associated with multiples nodes', function (assert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
test('a node pool can be associated with multiples nodes', function (assert) {
test('a node pool can have multiples nodes', function (assert) {

Minor nit-picking, but I think this describes the expected relationship better.


assert.equal(nodes.length, 2);
assert.ok(nodes.includes(node1));
assert.ok(nodes.includes(node2));
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be worth checking that the inverse relationship is also set, so check that node1 and node2 belong to nodePool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good call!

ChaiWithJai and others added 2 commits May 31, 2023 09:38
* chore: setup mirage models + factory hooks

* Acceptance test hardening to avoid faker/mirage randomness

---------

Co-authored-by: Phil Renaud <phil@riotindustries.com>
Co-authored-by: Luiz Aoqui <luiz@hashicorp.com>
@ChaiWithJai ChaiWithJai merged commit e3a9af6 into 17292/node-pool-ember-data May 31, 2023
3 of 4 checks passed
@ChaiWithJai ChaiWithJai deleted the 17293/mirage branch May 31, 2023 13:39
lgfa29 added a commit that referenced this pull request Jun 21, 2023
* refact:  update models for relationship

* chore: setup mirage models + factory hooks (#17321)

* chore: setup mirage models + factory hooks

* Acceptance test hardening to avoid faker/mirage randomness

---------

Co-authored-by: Phil Renaud <phil@riotindustries.com>

* chore:  update comment

Co-authored-by: Luiz Aoqui <luiz@hashicorp.com>

---------

Co-authored-by: Phil Renaud <phil@riotindustries.com>
Co-authored-by: Luiz Aoqui <luiz@hashicorp.com>
lgfa29 added a commit that referenced this pull request Jun 21, 2023
* refact:  update models for relationship

* chore: setup mirage models + factory hooks (#17321)

* chore: setup mirage models + factory hooks

* Acceptance test hardening to avoid faker/mirage randomness

---------

Co-authored-by: Phil Renaud <phil@riotindustries.com>

* chore:  update comment

Co-authored-by: Luiz Aoqui <luiz@hashicorp.com>

---------

Co-authored-by: Phil Renaud <phil@riotindustries.com>
Co-authored-by: Luiz Aoqui <luiz@hashicorp.com>
lgfa29 added a commit that referenced this pull request Jun 22, 2023
* refact:  update models for relationship

* chore: setup mirage models + factory hooks (#17321)

* chore: setup mirage models + factory hooks

* Acceptance test hardening to avoid faker/mirage randomness

---------

Co-authored-by: Phil Renaud <phil@riotindustries.com>

* chore:  update comment

Co-authored-by: Luiz Aoqui <luiz@hashicorp.com>

---------

Co-authored-by: Phil Renaud <phil@riotindustries.com>
Co-authored-by: Luiz Aoqui <luiz@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/node-pools Issues related to node pools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants