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

[MM-21123] Add Labels to upstream Marketplace response #13425

Merged
merged 11 commits into from Jan 14, 2020

Conversation

hanzei
Copy link
Contributor

@hanzei hanzei commented Dec 18, 2019

Summary

When fetching the Plugins from upstream Marketplace include also the fetched labels.

Ticket Link

https://mattermost.atlassian.net/browse/MM-21123
https://mattermost.atlassian.net/browse/MM-21611

Related Pull Requests

@hanzei hanzei added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Dec 18, 2019
@hanzei hanzei added this to the v5.20.0 milestone Dec 18, 2019
@hanzei hanzei changed the title Add Labels to upstream Marketplace response [MM-21123] [MM-21611] Add Labels to upstream Marketplace response Jan 8, 2020
@hanzei hanzei changed the title [MM-21123] [MM-21611] Add Labels to upstream Marketplace response [MM-21123] Add Labels to upstream Marketplace response Jan 8, 2020
@hanzei hanzei requested a review from lieut-data January 8, 2020 10:38
@hanzei
Copy link
Contributor Author

hanzei commented Jan 8, 2020

@lieut-data Re-requesting your review also I also included the changes needed for https://mattermost.atlassian.net/browse/MM-21611

app/plugin.go Outdated
Comment on lines 505 to 508
Labels: []model.MarketplaceLabel{{
Name: "Local",
Description: "This plugin is not listed in the marketplace but was installed manually.",
}},
Copy link
Member

Choose a reason for hiding this comment

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

This should be added only if the EnableRemoteMarketplace is enabled though. However, this setting is being introduced in this PR: #13449

What should be the best approach here to synch this? After you merge this maybe I can update the other PR to add the condition there

Copy link
Contributor Author

@hanzei hanzei Jan 9, 2020

Choose a reason for hiding this comment

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

Well, let's see what PR goes in first 😉 When can update the other one then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marianunez I've updated the #13449 in 1035471. I hope this was fine to you.

@hanzei hanzei requested a review from prapti January 9, 2020 20:06
@hanzei
Copy link
Contributor Author

hanzei commented Jan 10, 2020

Similar to mattermost/mattermost-marketplace#31 (comment), I also added a commit here in 6376e83

Copy link
Contributor

@alifarooq0 alifarooq0 left a comment

Choose a reason for hiding this comment

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

app/plugin.go Show resolved Hide resolved
@hanzei hanzei removed the 2: Dev Review Requires review by a developer label Jan 13, 2020
app/plugin.go Outdated Show resolved Hide resolved
Co-Authored-By: Jesse Hallam <jesse.hallam@gmail.com>
@prapti
Copy link

prapti commented Jan 13, 2020

@hanzei Executing curl localhost:8085/api/v1/health does not return info about the "OFFICIAL" tag. Was this change supposed to go in with this PR? The rest looks good.
The following is what gets returned for the curl command:

>curl localhost:8085/api/v1/health
>{"status":"pass","version":"1","releaseID":"","details":{"buildInfo":{"buildHash":"b47d3173b87aac83a20020ea0a13e5c1c71a83a8","buildHashShort":"b47d317"}},"description":"The stateless HTTP service backing the Mattermost marketplace"}

@lieut-data
Copy link
Member

@prapti, did you mean to test against the health endpoint? That will just return information about the marketplace server itself.

It looks like you have the right commits, but you should query /api/v1/plugins instead.

@prapti
Copy link

prapti commented Jan 13, 2020

@lieut-data I was testing based on the steps mentioned in this ticket. /api/v1/plugins does return proper info about the OFFICIAL tag.
@hanzei was the test step meant to query against /api/v1/plugins instead?

@hanzei
Copy link
Contributor Author

hanzei commented Jan 13, 2020

Good catch @prapti 👍

I've updated the QA steps in the ticket.

Copy link

@prapti prapti left a comment

Choose a reason for hiding this comment

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

Tested and verified on local instance that OFFICIAL and LOCAL tags show up for official Marketplace plugins and manually uploaded ones appropriately.
Some of the concerns regarding showing LOCAL plugins only when Enable Remote Marketplace is enabled will be tested during feature testing.

@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request Do Not Merge Should not be merged until this label is removed and removed 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Jan 13, 2020
@hanzei hanzei self-assigned this Jan 13, 2020
@hanzei hanzei removed the Do Not Merge Should not be merged until this label is removed label Jan 14, 2020
@hanzei hanzei merged commit 1f3f8fb into master Jan 14, 2020
@hanzei hanzei deleted the MM-21123_offical-labels branch January 14, 2020 06:16
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jan 17, 2020
@hanzei hanzei removed their assignment Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants