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

Change the update status as enum and rename Type into UpdateType in Update model #71

Merged
merged 2 commits into from
Oct 15, 2020
Merged

Change the update status as enum and rename Type into UpdateType in Update model #71

merged 2 commits into from
Oct 15, 2020

Conversation

ppamorim
Copy link
Contributor

@ppamorim ppamorim commented Oct 10, 2020 •
edited
Loading

This PR transforms the status result from the Update struct into an enum. This change is needed since the server has a stable API and the user doesn't want to keep comparing strings in the SDK.

@ppamorim ppamorim added enhancement New feature or request breaking-change The related changes are breaking for the users labels Oct 10, 2020
@ppamorim ppamorim self-assigned this Oct 10, 2020
@ppamorim ppamorim linked an issue Oct 10, 2020 that may be closed by this pull request
Replace Update.Result.status with enum values #55
Closed
@ppamorim
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Oct 10, 2020
@bors
Try #71:
1cad947
@bors
Copy link
Contributor

bors bot commented Oct 10, 2020

try

Build succeeded:

curquiza
curquiza reviewed Oct 12, 2020
View reviewed changes
Sources/MeiliSearch/Model/Update.swift Outdated Show resolved Hide resolved
Sources/MeiliSearch/Model/Update.swift Outdated Show resolved Hide resolved
Sources/MeiliSearch/Model/Update.swift Outdated Show resolved Hide resolved
Tests/MeiliSearchIntegrationTests/UpdatesTests.swift Outdated Show resolved Hide resolved
@curquiza
Copy link
Member

Great test added! 👍

@curquiza
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Oct 12, 2020
@bors
Try #71:
ba601e0
@bors
Copy link
Contributor

bors bot commented Oct 12, 2020

try

Build failed:

@curquiza curquiza changed the title Result status as enum Update status as enum Oct 12, 2020
@ppamorim
Copy link
Contributor Author

I need to rebase this PR since the test is failing.

@ppamorim
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Oct 12, 2020
@bors
Try #71:
fb06fbe
@bors
Copy link
Contributor

bors bot commented Oct 12, 2020

try

Build succeeded:

@curquiza curquiza changed the title Update status as enum Change the update status as enum Oct 13, 2020
bidoubiwa
bidoubiwa suggested changes Oct 13, 2020
View reviewed changes
Tests/MeiliSearchUnitTests/UpdatesTests.swift Outdated Show resolved Hide resolved
Tests/MeiliSearchUnitTests/UpdatesTests.swift Outdated Show resolved Hide resolved
Sources/MeiliSearch/Model/Update.swift Outdated Show resolved Hide resolved
@ppamorim ppamorim mentioned this pull request Oct 14, 2020
Merged
@bors
Copy link
Contributor

bors bot commented Oct 14, 2020

Build failed:

@curquiza
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Oct 14, 2020
@bors
Try #71:
31f6c61
@bors
Copy link
Contributor

bors bot commented Oct 14, 2020

try

Build failed:

@curquiza
Copy link
Member

There is clearly a problem when installing MeiliSearch on the CI with brew... I don't have any issues on my local machine. And you guys @eskombro @ppamorim @bidoubiwa?

@eskombro
Copy link
Member

eskombro commented Oct 14, 2020 •
edited
Loading

There is clearly a problem when installing MeiliSearch on the CI with brew... I don't have any issues on my local machine. And you guys @eskombro @ppamorim @bidoubiwa?

I have no issues. Installing meilisearch with brew works fine for me!

We could install it on the CI with curl -L https://install.meilisearch.com | sh instead

brew update is loooooong :)

@curquiza
Copy link
Member

Yes indeed we should change!
The script is done by humans, I would rather use docker as we do in the other SDKs, it works fine:
https://github.com/meilisearch/meilisearch-php/blob/ced74248be12f3ca594ce115419cf28d3ef6d609/.github/workflows/tests.yml#L35-L36

@eskombro
Copy link
Member

eskombro commented Oct 14, 2020 •
edited
Loading

+1 for docker

Sorry @ppamorim , I know this is not necessary related to your contribution :)

@curquiza curquiza mentioned this pull request Oct 14, 2020
Closed
@curquiza
Copy link
Member

curquiza commented Oct 14, 2020 •
edited
Loading

Oh I remember, we spent a lot of time with Charlotte about that! Sorry again Pedro because it's not directly related to the issue.

We used to use the installation script, but we don't know why, it fails on macos-latest (only on CI. It works perfectly locally). See meilisearch/meilisearch#908
Docker is not installed on the macos-latest CI.
So, brew was the latest option...

Let's wait, it's maybe a GitHub issue... It worked a few hours ago.

@eskombro
Copy link
Member

eskombro commented Oct 14, 2020 •
edited
Loading

Let's wait, it's maybe a GitHub issue... It worked a few hours ago.

Ok! Indeed it probably is a connection/authentication issue coming from GitHub. So if it keeps doing the same we can create a new issue (for the swift CI specifically) and test/solve everything there!

@ppamorim
Copy link
Contributor Author

@eskombro @curquiza Same error here: actions/runner-images#1811

@ppamorim ppamorim dismissed stale reviews from curquiza and bidoubiwa via dbb5782 October 14, 2020 17:24
@ppamorim
Copy link
Contributor Author

Sorted with this change:

run: |
    brew uninstall openssl@1.0.2t
    brew uninstall python@2.7.17
    brew untap local/openssl
    brew untap local/python2
    brew update

@curquiza
Copy link
Member

If it's a temporary fix I would rather not add it.
If it's a definitive fix, I would rather have it in another PR please 🙂

@ppamorim ppamorim mentioned this pull request Oct 14, 2020
Closed
@curquiza curquiza changed the title Change the update status as enum Change the update status as enum and rename Type into UpdateType in Update model Oct 15, 2020
@curquiza
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Oct 15, 2020
@bors
Try #71:
e294c39
@bors
Copy link
Contributor

bors bot commented Oct 15, 2020

try

Build succeeded:

@curquiza
Copy link
Member

bors merge

@bors
Copy link
Contributor

bors bot commented Oct 15, 2020

Build succeeded:

@bors bors bot merged commit e21f2aa into master Oct 15, 2020
@bors bors bot deleted the feature/updateResultStatus branch October 15, 2020 07:37
@ppamorim ppamorim mentioned this pull request Oct 23, 2020
Merged
bors bot added a commit that referenced this pull request Oct 23, 2020
@bors @ppamorim
Merge #84
c7c35fb 
84: Fix CI r=curquiza a=ppamorim

I don't see they fixing this issue, we are getting stuck due to this problem.

Fixes crash in CI reported here: actions/runner-images#1811

Crash happening in the PR #71.

Co-authored-by: Pedro Paulo de Amorim <pp.amorim@hotmail.com>
Co-authored-by: Pedro Paulo Amorim <pp.amorim@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@curquiza curquiza curquiza approved these changes

@bidoubiwa bidoubiwa bidoubiwa left review comments

Assignees

@ppamorim ppamorim

Labels
breaking-change The related changes are breaking for the users enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace Update.Result.status with enum values
4 participants
@ppamorim @curquiza @eskombro @bidoubiwa