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

Ml calendar apis #3569

Merged
merged 11 commits into from
Mar 6, 2019
Merged

Ml calendar apis #3569

merged 11 commits into from
Mar 6, 2019

Conversation

codebrain
Copy link
Contributor

@codebrain codebrain commented Feb 11, 2019

Implementation of the 6.4.0 ML calendar APIs - see here: #3552

Requires port to master

@codebrain codebrain requested a review from russcam February 11, 2019 23:55
@codebrain codebrain self-assigned this Feb 11, 2019
@codebrain codebrain mentioned this pull request Feb 12, 2019
48 tasks
@Mpdreamz
Copy link
Member

@codebrain mind retargetting this against 6.6 and investigate the CI failures? 👍

@russcam russcam changed the base branch from 6.x to 6.6 March 5, 2019 05:24
@russcam
Copy link
Contributor

russcam commented Mar 5, 2019

Changed base to 6.6

Copy link
Contributor

@russcam russcam left a comment

Choose a reason for hiding this comment

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

LGTM 👍 left a few small comments.

I added a fix for failing unit tests related to there being no event id when unit tests are run.

public partial interface IDeleteCalendarJobResponse : IResponse
{
[JsonProperty("calendar_id")]
Id CalendarId { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we typically return Nest types like Id on responses, or leave them as primitive types e.g. string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, will change to string - I think that is better and will likely play nicer with a different serialiser.

public partial interface IElasticClient
{
/// <summary>
/// Retrieves configuration for machine learning jobs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Retrieves calendar configuration information for machine learning jobs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

public partial interface IElasticClient
{
/// <summary>
/// Instantiates a machine learning calendar.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe Creates might be better here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@codebrain codebrain merged commit c5dea4a into 6.6 Mar 6, 2019
@codebrain
Copy link
Contributor Author

Ported to master

@codebrain
Copy link
Contributor Author

Subsequent commit made to address other types in response objects, not merged as part of this commit. This was also ported to master:
06a4d3e

@codebrain codebrain deleted the ml-calendar-apis branch March 7, 2019 02:47
russcam pushed a commit that referenced this pull request Mar 20, 2019
russcam pushed a commit that referenced this pull request Mar 21, 2019
russcam pushed a commit that referenced this pull request Apr 2, 2019
(cherry picked from commit 2f040c4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants