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

Node resource CRUD #85

Merged
merged 7 commits into from
Jul 1, 2016
Merged

Node resource CRUD #85

merged 7 commits into from
Jul 1, 2016

Conversation

alxngyn
Copy link
Contributor

@alxngyn alxngyn commented May 27, 2016

fixes issue #22

Changes in this PR.

  • Added spec tests for /node endpoint
  • Added /node endpoint similar to project and client in app.rb

Testing this PR.

  1. Start container
  2. Run rake spec and see tests pass
  3. Side note: going to localhost:4567/nodes will result in a blank page since we did not run the scheduler to fill the DB with data

Expected Output.

$ rake spec
[ should pass ]

@alxngyn
Copy link
Contributor Author

alxngyn commented May 27, 2016

one rake spec test will fail without name validation
Blocked by #86

@alxngyn
Copy link
Contributor Author

alxngyn commented Jun 20, 2016

rebased with develop, tests should now all pass

pop
pop reviewed Jun 28, 2016
View reviewed changes
routes/nodes.rb
# recieve new node
node = NodeResource.create(project_id: Iam.settings.DB[:projects]
.where(name: params[:project_name])
.get(:id) || node.project_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the || node.project_id work if we're creating the node being referred to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, that won't work.
It should be OR'd with a blank('') if no project is found

@pop
Copy link
Contributor

pop commented Jun 28, 2016

This looks pretty good. The tests seem to cover the basics and the implementation is good. The tests seem to be covering the NodeResource model, do we have any interest in testing the endpoints?

For instance we sould use the endpoint and checking the database to see if the right thing happened? I don't think we've been doing this but I think we really should.

Thoughts @Kennric @alxngyn @athai @subnomo

@alxngyn
Copy link
Contributor Author

alxngyn commented Jul 1, 2016

Added ~9 new tests for the /node endpoints.
rake spec should all pass

@pop
Copy link
Contributor

pop commented Jul 1, 2016

@alxngyn After merging the changes from #87 I am able to run the app and it works seamlessly, so when we merge things should work themselves out. If you want we can rebase with develop before merging just to be sure, I don't care either way. The tests pass and seem thorough.
👍 from me

@alxngyn alxngyn merged commit 2290ddb into develop Jul 1, 2016
@Kennric Kennric deleted the alxngyn/node-crud branch June 20, 2017 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Ready for Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@alxngyn @pop