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

Set up plugin boilerplate & placeholder pages #3

Merged
merged 3 commits into from
Sep 5, 2023
Merged

Set up plugin boilerplate & placeholder pages #3

merged 3 commits into from
Sep 5, 2023

Conversation

ohltyler
Copy link
Member

@ohltyler ohltyler commented Aug 31, 2023 •
edited
Loading

Description

What it looks like:
screen-capture (10).webm

Issues Resolved

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@ohltyler ohltyler added the infra Changes to infrastructure, testing, CI/CD, etc. label Aug 31, 2023
joshpalis
joshpalis reviewed Sep 5, 2023
View reviewed changes
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
BSFishy
BSFishy reviewed Sep 5, 2023
View reviewed changes
package.json
Comment on lines +13 to +16
"plugin-helpers": "../../scripts/use_node ../../scripts/plugin_helpers",
"osd": "../../scripts/use_node ../../scripts/osd",
"opensearch": "node ../../scripts/opensearch",
"lint": "node ../../scripts/eslint .",
Copy link

Choose a reason for hiding this comment

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

Why do some of these use use_node and others don't?

Also, plugins should also have a lint:style script, that calls ../../scripts/use_node ../../scripts/stylelint (which should be part of the CI workflow, along with lint:es)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll clean up the use_node vs. node differences, I referenced some of this from different existing plugins.

Thanks for the suggestions on the lint scripts - I'll work on that + adding a github workflow for them

Copy link
Member Author

Choose a reason for hiding this comment

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

@BSFishy I've updated and added a lint script in latest commit. For some reason there is an issue with that commit showing up as part of this PR, checking on if that is a newly-public-repo config that needs updated or related to the list of incidents that've happened today

owaiskazi19
owaiskazi19 reviewed Sep 5, 2023
View reviewed changes
Copy link
Member

@owaiskazi19 owaiskazi19 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm very new to reviewing frontend PRs. Feel free to ignore my comment if not relevant.
Also, getting permission denied while viewing the screen capture.

public/app.tsx Show resolved Hide resolved
public/index.ts Show resolved Hide resolved
public/plugin.ts Show resolved Hide resolved
public/utils/index.ts Show resolved Hide resolved
@ohltyler
Copy link
Member Author

ohltyler commented Sep 5, 2023

I'm very new to reviewing frontend PRs. Feel free to ignore my comment if not relevant. Also, getting permission denied while viewing the screen capture.

Weird, it was working originally but now I'm seeing the same permission denied. Just re-uploaded and it's available again

@owaiskazi19
Copy link
Member

owaiskazi19 commented Sep 5, 2023 •
edited
Loading

Weird, it was working originally but now I'm seeing the same permission denied. Just re-uploaded and it's available again

Do we need to install some github extension to view the video because I'm seeing this:

I was using Firefox, able to see it on Chrome.

Screenshot 2023-09-05 at 12 59 09 PM

@ohltyler
Copy link
Member Author

ohltyler commented Sep 5, 2023

@BSFishy I believe the issue is that my forked repo was forked before the repo was public. It is not showing as an existing fork on the repo now.

I will have a follow up PR with the added commits in my new forked repo

@ohltyler ohltyler merged commit eab9065 into opensearch-project:main Sep 5, 2023
7 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 5, 2023
@ohltyler @github-actions
Set up plugin boilerplate & placeholder pages (#3)
bd3edb5 
Signed-off-by: Tyler Ohlsen <[email protected]>
(cherry picked from commit eab9065)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Sep 5, 2023
Merged
@ohltyler ohltyler mentioned this pull request Sep 5, 2023
Merged
4 tasks
ohltyler added a commit that referenced this pull request Sep 5, 2023
@opensearch-trigger-bot @ohltyler
Set up plugin boilerplate & placeholder pages (#3) (#24)
4f32a2a 
Signed-off-by: Tyler Ohlsen <[email protected]>
(cherry picked from commit eab9065)

Co-authored-by: Tyler Ohlsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owaiskazi19 owaiskazi19 owaiskazi19 left review comments

@BSFishy BSFishy BSFishy left review comments

@joshpalis joshpalis joshpalis approved these changes

@dbwiddis dbwiddis Awaiting requested review from dbwiddis dbwiddis is a code owner

@amitgalitz amitgalitz Awaiting requested review from amitgalitz amitgalitz is a code owner

@jackiehanyang jackiehanyang Awaiting requested review from jackiehanyang jackiehanyang is a code owner

Assignees
No one assigned
Labels
backport 2.x dependencies infra Changes to infrastructure, testing, CI/CD, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ohltyler @owaiskazi19 @BSFishy @joshpalis