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

Populate project without update job in Tower #82

Merged
merged 2 commits into from
Jul 20, 2018

Conversation

Glutexo
Copy link
Member

@Glutexo Glutexo commented May 10, 2018

Spec examples introduced by #72 require one project with an update job and another without one. Use another_repo for the latter case. Alter the populate_tower Rake task to delete the update job after the initial update finishes.

Merging this PR does not make sense before merging other PRs:

@miq-bot add_reviewer @jameswnl

@miq-bot miq-bot requested a review from jameswnl May 10, 2018 12:51
@Glutexo Glutexo force-pushed the populate_project_without_update_job branch 2 times, most recently from 032ecb3 to d8a88b9 Compare May 24, 2018 14:04
@Glutexo Glutexo changed the title Populate project without update job in Tower [wip] Populate project without update job in Tower May 29, 2018
@miq-bot miq-bot added the wip label May 29, 2018
@miq-bot
Copy link
Member

miq-bot commented May 31, 2018

This pull request is not mergeable. Please rebase and repush.

@Glutexo Glutexo changed the title [wip] Populate project without update job in Tower Populate project without update job in Tower Jun 4, 2018
@miq-bot miq-bot removed the wip label Jun 4, 2018
@Glutexo Glutexo force-pushed the populate_project_without_update_job branch from d8a88b9 to 9aff831 Compare June 5, 2018 09:58
@Glutexo
Copy link
Member Author

Glutexo commented Jun 5, 2018

Rebased on current state of #72.

@Glutexo
Copy link
Member Author

Glutexo commented Jun 7, 2018

@miq-bot add_label unmergeable

@Glutexo
Copy link
Member Author

Glutexo commented Jun 7, 2018

@miq-bot add_label pending core

@jameswnl jameswnl closed this Jun 7, 2018
@jameswnl jameswnl reopened this Jun 7, 2018
Copy link
Contributor

@jameswnl jameswnl left a comment

Choose a reason for hiding this comment

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

Please rip out the duplicated changes with your other PR(s)

@miq-bot
Copy link
Member

miq-bot commented Jun 7, 2018

This pull request is not mergeable. Please rebase and repush.

@Glutexo Glutexo force-pushed the populate_project_without_update_job branch 2 times, most recently from 76a20e0 to 8baf65c Compare June 8, 2018 10:52
@Glutexo
Copy link
Member Author

Glutexo commented Jun 8, 2018

I rebased to the current master, so it doesn’t contain any duplicate commits. Still I’d like to note that this PR introduces changes that are only useful after #72 gets into master. Merging this PR without #72 doesn’t make much sense.

Is there maybe some label, better that unmergeable to stick to such PRs?

* Create a failed_repo to test saving last update stdout.
* Create a jobless_repo to test behavior of loading a project without
  an update job. Delete the job after it finishes upon project creation.
@Glutexo Glutexo force-pushed the populate_project_without_update_job branch from 8baf65c to 361bff5 Compare June 21, 2018 10:06
@Glutexo
Copy link
Member Author

Glutexo commented Jun 21, 2018

The last update job is used to capture its stdout. This output is now stored only if the update fails. [1], [2] To test this, we need more than one project: 3

  • A project with successful update.
  • A project with failed update.
  • A project without update.

Updated the Rake task to create those projects. Removed another_repo, any of the new ones (jobless_repo or failed_repo) can be used instead by #93 and #92.

@jameswnl might want to review this change.

Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

I'd like to see @jameswnl 's approval on this. Otherwise my only concern are the messy Hashes, which seems to me rather unreadible than anything else. I'll take it over this PR and update it.


# create a job_template
uri = '/api/v1/job_templates/'
data = {"name" => 'hello_template', "description" => "test job", "job_type" => "run", "project" => project['id'], "playbook" => "hello_world.yml", "credential" => machine_credential['id'], "cloud_credential" => aws_credential['id'], "network_credential" => network_credential['id'], "inventory" => inventory['id'], "organization" => organization['id']}
data = {"name" => 'hello_template', "description" => "test job", "job_type" => "run", "project" => hello_project['id'], "playbook" => "hello_world.yml", "credential" => machine_credential['id'], "cloud_credential" => aws_credential['id'], "network_credential" => network_credential['id'], "inventory" => inventory['id'], "organization" => organization['id']}
Copy link
Member

Choose a reason for hiding this comment

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

I'll try to convert this into proper Ruby hashes. It still gets serialized as JSON on send so, I don't see a reason why to keep it messy.

Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

I did a cleanup on that file, it was painful to read it. I know that's not the point of this PR, though I had to do it...

@tumido
Copy link
Member

tumido commented Jun 26, 2018

The other PR #72 marked as "merge before" doesn't necessarily block this one. The dependency is rather a use-case for the environment prepared by this PR. We shouldn't need to wait.

@tumido tumido force-pushed the populate_project_without_update_job branch from a4fe4a1 to 49e4ace Compare June 26, 2018 15:18
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

:neckbeard: 🎆 🎊 🎸 🌈

@miq-bot
Copy link
Member

miq-bot commented Jun 26, 2018

Some comments on commits Glutexo/manageiq-providers-ansible_tower@361bff5~...49e4ace

lib/tasks_private/spec_helper.rake

  • ⚠️ - 397 - Detected puts. Remove all debugging statements.
  • ⚠️ - 408 - Detected puts. Remove all debugging statements.
  • ⚠️ - 81 - Detected puts. Remove all debugging statements.
  • ⚠️ - 90 - Detected puts. Remove all debugging statements.

@miq-bot
Copy link
Member

miq-bot commented Jun 26, 2018

Checked commits Glutexo/manageiq-providers-ansible_tower@361bff5~...49e4ace with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@Fryguy
Copy link
Member

Fryguy commented Jun 27, 2018

In the future, please keep unrelated rubocop changes to a separate PR.

@tumido
Copy link
Member

tumido commented Jun 28, 2018

I know. We had a discussion about that here a4fe4a1#commitcomment-29502367 so we decided to keep it this way...

@tumido
Copy link
Member

tumido commented Jul 19, 2018

@jameswnl, can you review this once more, please? Are there any concerns which should be addressed?

@jameswnl jameswnl merged commit bff407a into ManageIQ:master Jul 20, 2018
@bronaghs bronaghs added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants