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

Place all the OpenShift related logic at one place #56

Merged
merged 5 commits into from
Aug 28, 2018
Merged

Place all the OpenShift related logic at one place #56

merged 5 commits into from
Aug 28, 2018

Conversation

fridex
Copy link
Contributor

@fridex fridex commented Aug 8, 2018

No description provided.

@fridex fridex requested a review from goern August 8, 2018 11:12
@sesheta sesheta added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 8, 2018
@ghost
Copy link

ghost commented Aug 8, 2018

Build failed.

@sesheta sesheta added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 8, 2018
@sesheta sesheta added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 8, 2018
@ghost
Copy link

ghost commented Aug 8, 2018

Build succeeded.

@sesheta sesheta added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 8, 2018
@ghost
Copy link

ghost commented Aug 8, 2018

Build succeeded.

@sesheta sesheta added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 8, 2018
@ghost
Copy link

ghost commented Aug 8, 2018

Build succeeded.

This was referenced Aug 8, 2018
Merged
Merged
@ghost
Copy link

ghost commented Aug 8, 2018

Build failed.

goern
goern reviewed Aug 8, 2018
View reviewed changes
thoth/common/openshift.py Outdated

def _oc_process(self, namespace: str, template: dict) -> dict:
"""Process the given template in OpenShift."""
# This does not work - see issue reported upstream:
Copy link
Member

Choose a reason for hiding this comment

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

could you put some @todo here and lets see what happends?!

thoth/common/openshift.py
'value': str(parameter_value)
})

def run_sync(self, force_analysis_results_sync: bool = False, force_solver_results_sync: bool = False) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

this whole section of preparing the pod/job template looks complicated, why is that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It runs job and reuses all the configuration of cronjob.

@todo
Copy link

todo bot commented Aug 28, 2018

rewrite to OpenShift rest client once it will support it.

common/thoth/common/openshift.py

Lines 147 to 152 in 4a94d92

# TODO: rewrite to OpenShift rest client once it will support it.
endpoint = "{}/api/v1/namespaces/{}/pods/{}/log".format(
self.kubernetes_api_url,
namespace,
pod_id
)

This comment was generated by todo based on a TODO comment in 4a94d92 in #56. cc @fridex.

@fridex
Copy link
Contributor Author

fridex commented Aug 28, 2018

@goern I would like to proceed with this one, but it looks like CI does not report status anymore. Could you take a look?

@fridex
Copy link
Contributor Author

fridex commented Aug 28, 2018

recheck

@fridex
Copy link
Contributor Author

fridex commented Aug 28, 2018

recheck

@ghost
Copy link

ghost commented Aug 28, 2018

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@sesheta sesheta added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 28, 2018
@fridex
Copy link
Contributor Author

fridex commented Aug 28, 2018

I will merge this to continue with work. If there are any issues/objections/suggestions I will incorporate them :)

@ghost
Copy link

ghost commented Aug 28, 2018

Build failed.

@fridex
Copy link
Contributor Author

fridex commented Aug 28, 2018

recheck

@ghost
Copy link

ghost commented Aug 28, 2018

Build succeeded.

@fridex fridex merged commit 5e0f7ac into thoth-station:master Aug 28, 2018
@fridex fridex deleted the openshift-extras branch August 28, 2018 16:59
@todo todo bot mentioned this pull request Aug 28, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goern goern goern left review comments

Assignees
No one assigned
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fridex @goern @sesheta