feat/add fishtape option to fish feature #25

edouard-lopez · 2022-11-07T10:00:25Z

Adding fisher_plugins to @guiyomh's fish feature.

Usage

"features": {
    "ghcr.io/meaningful-ooo/devcontainer-features/fish:1": {
        "fisher_plugins": "jorgebucaran/fishtape pure-fish/pure"
    }
}

Options

Options Id	Description	Type	Default Value
`fisher_plugins`	Install Fish's plugins using `fisher`	string	`""`

test default devcontainer (without fishtape) and one with the option enable

eitsupi · 2022-11-07T10:55:11Z

Thank you for working on this.

But I don't think we should add any more plugins here to be installed by fisher. (I don't think that's a sustainable approach)

Users can install their favorite plug-ins in an instant by simply copying the ~/.config/fish/fish_plugins file created by fisher into the container and executing the fisher update command.

andreiborisov · 2022-11-07T11:18:48Z

@edouard-lopez hello, friend! 👋🏻

But I don't think we should add any more plugins here to be installed by Fisher (I don't think that's a sustainable approach).

Maybe we can use an array for plugins to install instead? @eitsupi @edouard-lopez what do you think?

eitsupi · 2022-11-07T11:23:05Z

Maybe we can use an array for plugins to install instead?

I think adding an array would be a good idea. 👍

eitsupi · 2022-11-07T11:24:28Z

src/fish/README.md

+
+[fisher]: https://github.com/jorgebucaran/fisher
+[fishtape]: https://github.com/jorgebucaran/fishtape


Do not edit README.md directly as it is automatically generated from devcontainer-feature.json and NOTES.md.

andreiborisov · 2022-11-07T11:25:18Z

I think adding an array would be a good idea. 👍

The only issue, there is no array type for options: https://containers.dev/implementors/features/#options-property 😓

eitsupi · 2022-11-07T11:29:24Z

A string type option that allows specifying plug-ins as a string separated by spaces or commas would be suitable.

"plugins": "jorgebucaran/nvm.fish meaningful-ooo/sponge"

andreiborisov · 2022-11-07T11:29:34Z

@edouard-lopez is this PR time-sensitive? There is a proposal to add arrays to the spec: devcontainers/spec#57. I prefer to wait for it instead of hardcoding specific plugins.

edouard-lopez · 2022-11-07T11:30:36Z

Array sounds good to me, it's not time-sensitive so we can wait or use @eitsupi workaround

andreiborisov · 2022-11-07T11:32:36Z

A string type option that allows specifying plug-ins as a string separated by spaces or commas would be suitable.

Good idea! Let's wait for a reply from the spec maintainers and decide if we want to use this workaround or wait for the proper array support.

…rated

edouard-lopez · 2022-11-07T16:01:52Z

I updated the code to support a fisher_plugins options and added 2 scenarios to test the options.

BTW, Are you aware of solution to test a single scenario with devcontainer features test?

andreiborisov · 2022-11-07T19:10:58Z

HAW-29 Option to install Fisher plugins

edouard-lopez · 2022-11-08T09:16:56Z

@andreiborisov, are you aware the linear links are not accessible to others?

eitsupi

Thank you for working on this.
I think the updates regarding the development environment, such as justfile commits, should be separated from the PR, but anyway some small comments regarding the Feature.

eitsupi · 2022-11-08T10:51:08Z

src/fish/devcontainer-feature.json

@@ -9,6 +9,11 @@
      "type": "boolean",
      "default": true,
      "description": "Install Fisher plugin manager"
+    },
+    "fisher_plugins": {


It should be camelCase.

Suggested change

"fisher_plugins": {

"fisherPlugins": {

eitsupi · 2022-11-08T10:52:43Z

src/fish/install.sh

@@ -118,6 +119,24 @@ if [ "${FISHER}" = "true" ]; then
  fish -c "fisher -v"
 fi

+# Install Fisher plugins
+if [ -n "${FISHER_PLUGINS}" ]; then


fisher should be installed.

Suggested change

if [ -n "${FISHER_PLUGINS}" ]; then

if [ "${FISHER}" = "true" ] && [ -n "${FISHERPLUGINS}" ]; then

eitsupi · 2022-11-08T10:53:07Z

src/fish/install.sh

@@ -1,6 +1,7 @@
 #!/usr/bin/env bash

 FISHER=${FISHER:-"true"}
+FISHER_PLUGINS=${FISHER_PLUGINS:-""}


Suggested change

FISHER_PLUGINS=${FISHER_PLUGINS:-""}

FISHERPLUGINS=${FISHERPLUGINS:-""}

I propose to go with the same approach maintainers currently use in https://github.com/devcontainers/features: devcontainers/spec#57 (comment)

I think it's a little bit more readable in options and they can effectively migrate us later when the proper arrays will become supported:

BASH_ARRAY=( ${COMMA_SEPARATED_OPTION//,/ } )

Vote with 👍 or 👎, please.

eitsupi · 2022-11-08T11:03:18Z

Are you aware of solution to test a single scenario with devcontainer features test?

Running only scenario tests can be done as follows, but I believe that running only one scenario is not supported.
We will probably need to rewrite the json file using jq.

devcontainer-features/.github/workflows/test.yaml

Line 70 in 2177b40

    
           run: devcontainer features test -f ${{ matrix.features }} --skip-autogenerated .

andreiborisov · 2022-11-08T12:07:33Z

Are you aware the Linear links are not accessible to others?

@edouard-lopez yes, I've wanted to invite you via Twitter (I've lost your Telegram contact 🥲), but it might be that your notifications for DMs are disabled.

Ping me, please, on Telegram or Twitter 🤙🏻

andreiborisov

Thank you for working on this PR! 🙏🏻

andreiborisov

We don't have proper commit guidelines yet: https://linear.app/meaningful/issue/SYM-5/commit-style-guide, so I apologize for the chaos.

Although we might arrive at a style similar to what you've used in your commits, right now it's pure gitmoji without scope or type (the type is actually redundant since gitmoji already serves this purpose).

I would love to hear your thoughts on all this if you want to discuss it 🤙🏻

andreiborisov · 2022-11-10T17:49:27Z

CONTRIBUTING.md

+## Install
+
+```console
+yarn install


We already use pnpm as it's faster compared to yarn.

Contributing guidelines is a huge topic for Meaningful, so they're developed separately to be used organization-wide: https://linear.app/meaningful/issue/GRA-8/contributing-guidelines

The goal is to be as modular and reusable as we possibly can.

andreiborisov · 2022-11-10T17:53:16Z

CONTRIBUTING.md

+Pass a `feature` and a `base image`, _e.g._:
+
+```fish
+just test fish mcr.microsoft.com/devcontainers/base:debian


There is also https://github.com/go-task/task which looks good. Before committing to something, we should compare those (sorry for being pedantic, I'd like those decisions to be organization-wide so it requires a certain level of scrupulousness).

andreiborisov · 2022-11-10T17:57:27Z

src/fish/install.sh

@@ -1,6 +1,7 @@
 #!/usr/bin/env bash

 FISHER=${FISHER:-"true"}
+FISHER_PLUGINS=${FISHER_PLUGINS:-""}


I propose to go with the same approach maintainers currently use in https://github.com/devcontainers/features: devcontainers/spec#57 (comment)

I think it's a little bit more readable in options and they can effectively migrate us later when the proper arrays will become supported:

BASH_ARRAY=( ${COMMA_SEPARATED_OPTION//,/ } )

Vote with 👍 or 👎, please.

andreiborisov · 2022-11-10T18:03:28Z

@eitsupi thank you for helping with the review, I was quite slow this time 🐢

andreiborisov · 2022-11-10T19:07:47Z

CONTRIBUTING.md

+## Install
+
+```console
+yarn install


Contributing guidelines is a huge topic for Meaningful, so they're developed separately to be used organization-wide: https://linear.app/meaningful/issue/GRA-8/contributing-guidelines

The goal is to be as modular and reusable as we possibly can.

eitsupi · 2022-11-11T14:52:47Z

Are you aware of solution to test a single scenario with devcontainer features test?

Running only scenario tests can be done as follows, but I believe that running only one scenario is not supported.

FYI devcontainers/cli#272

eitsupi · 2022-12-15T13:44:21Z

Any update on this?

edouard-lopez added 4 commits November 7, 2022 11:09

🧑‍💻 chore: add @devcontainers/cli to devDependencies

6dfeb99

📝 docs: init documentation with install step

f876434

✅ tests: add justfile to run tests and documentation

e27adf3

✨ feat(fish): add option to install fishtape

a2df261

test default devcontainer (without fishtape) and one with the option enable

edouard-lopez force-pushed the feat/add-fishtape-option-to-fish-feature branch from 29038ca to a2df261 Compare November 7, 2022 10:12

eitsupi reviewed Nov 7, 2022

View reviewed changes

edouard-lopez marked this pull request as draft November 7, 2022 11:29

edouard-lopez added 2 commits November 7, 2022 15:44

📝 docs: mirgate contributions guide to own docs as readme is autogene…

4173935

…rated

✨ refactor: replace fishtape option with fisher_plugins options

557a3c5

andreiborisov self-requested a review November 7, 2022 18:46

andreiborisov self-assigned this Nov 7, 2022

edouard-lopez marked this pull request as ready for review November 8, 2022 09:16

eitsupi requested changes Nov 8, 2022

View reviewed changes

andreiborisov reviewed Nov 10, 2022

View reviewed changes

andreiborisov requested changes Nov 10, 2022

View reviewed changes

andreiborisov marked this pull request as draft December 19, 2022 07:47

Bzybree92 mentioned this pull request Sep 16, 2024

fisher should be installed. #61

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat/add fishtape option to fish feature #25

feat/add fishtape option to fish feature #25

edouard-lopez commented Nov 7, 2022 •

edited

Loading

eitsupi commented Nov 7, 2022

andreiborisov commented Nov 7, 2022

eitsupi commented Nov 7, 2022

eitsupi Nov 7, 2022

andreiborisov commented Nov 7, 2022

eitsupi commented Nov 7, 2022

andreiborisov commented Nov 7, 2022

edouard-lopez commented Nov 7, 2022

andreiborisov commented Nov 7, 2022

edouard-lopez commented Nov 7, 2022

andreiborisov commented Nov 7, 2022

edouard-lopez commented Nov 8, 2022

eitsupi left a comment

eitsupi Nov 8, 2022

eitsupi Nov 8, 2022

eitsupi Nov 8, 2022

andreiborisov Nov 10, 2022 •

edited

Loading

eitsupi commented Nov 8, 2022

andreiborisov commented Nov 8, 2022 •

edited

Loading

andreiborisov left a comment

andreiborisov left a comment •

edited

Loading

andreiborisov Nov 10, 2022 •

edited

Loading

andreiborisov Nov 10, 2022 •

edited

Loading

andreiborisov Nov 10, 2022

andreiborisov Nov 10, 2022 •

edited

Loading

andreiborisov commented Nov 10, 2022

andreiborisov Nov 10, 2022 •

edited

Loading

eitsupi commented Nov 11, 2022

eitsupi commented Dec 15, 2022


		[fisher]: https://github.com/jorgebucaran/fisher
		[fishtape]: https://github.com/jorgebucaran/fishtape

	if [ -n "${FISHER_PLUGINS}" ]; then
	if [ "${FISHER}" = "true" ] && [ -n "${FISHERPLUGINS}" ]; then

	FISHER_PLUGINS=${FISHER_PLUGINS:-""}
	FISHERPLUGINS=${FISHERPLUGINS:-""}

feat/add fishtape option to fish feature #25

Are you sure you want to change the base?

feat/add fishtape option to fish feature #25

Conversation

edouard-lopez commented Nov 7, 2022 • edited Loading

Usage

Options

eitsupi commented Nov 7, 2022

andreiborisov commented Nov 7, 2022

eitsupi commented Nov 7, 2022

eitsupi Nov 7, 2022

Choose a reason for hiding this comment

andreiborisov commented Nov 7, 2022

eitsupi commented Nov 7, 2022

andreiborisov commented Nov 7, 2022

edouard-lopez commented Nov 7, 2022

andreiborisov commented Nov 7, 2022

edouard-lopez commented Nov 7, 2022

andreiborisov commented Nov 7, 2022

edouard-lopez commented Nov 8, 2022

eitsupi left a comment

Choose a reason for hiding this comment

eitsupi Nov 8, 2022

Choose a reason for hiding this comment

eitsupi Nov 8, 2022

Choose a reason for hiding this comment

eitsupi Nov 8, 2022

Choose a reason for hiding this comment

andreiborisov Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

eitsupi commented Nov 8, 2022

andreiborisov commented Nov 8, 2022 • edited Loading

andreiborisov left a comment

Choose a reason for hiding this comment

andreiborisov left a comment • edited Loading

Choose a reason for hiding this comment

andreiborisov Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

andreiborisov Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

andreiborisov Nov 10, 2022

Choose a reason for hiding this comment

andreiborisov Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

andreiborisov commented Nov 10, 2022

andreiborisov Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

eitsupi commented Nov 11, 2022

eitsupi commented Dec 15, 2022

edouard-lopez commented Nov 7, 2022 •

edited

Loading

andreiborisov Nov 10, 2022 •

edited

Loading

andreiborisov commented Nov 8, 2022 •

edited

Loading

andreiborisov left a comment •

edited

Loading

andreiborisov Nov 10, 2022 •

edited

Loading

andreiborisov Nov 10, 2022 •

edited

Loading

andreiborisov Nov 10, 2022 •

edited

Loading

andreiborisov Nov 10, 2022 •

edited

Loading