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

Include README.md as description #17

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

jmsktm
Copy link

@jmsktm jmsktm commented Apr 5, 2023

It's much easier to include description about the plugin in a markdown than in the spec itself. If we plumb it this way, it's more likely that the users will follow the same.

Fyi, with #3096 merged in, the markdowns are formatted pretty nicely as well.
Example: https://app.staging.signadot.com/resource-plugins/name/mariadb

Open to opinion.

Copy link
Member

@scott-cotton scott-cotton left a comment

Choose a reason for hiding this comment

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

LGTM

@daniel-de-vera
Copy link
Contributor

There a couple of things that I don't like about this approach (and/or about the changes introduced in https://github.com/signadot/signadot/pull/3096):

  1. The spec looks like text plain:

Screenshot from 2023-04-05 09-42-27

IMO, ideally it should be formatted as a YAML, something like:

Screenshot from 2023-04-05 09-46-41

That before https://github.com/signadot/signadot/pull/3096, at least had the gray background.

  1. I don't think the README.md without context (without being part of the plugins repo) makes full sense, for things like:

Screenshot from 2023-04-05 09-52-09

Those lines could easily confuse the user? what does that mean? where are those files (./k8s/mariadb-init.yaml, etc)?

  1. The addition of the README.md is making the RP spec, very "noisy", where the important things, like steps and scripts are hard to find:

Screenshot from 2023-04-05 09-55-24

@jmsktm
Copy link
Author

jmsktm commented Apr 5, 2023

@daniel-de-vera:

#1: The current design for yaml editor is temporary. I plan to use a different off-the-shelf component for displaying yaml afterwards. I had actually done that in one of the pull requests. But had to scrap that because the version update for webpack led to some conflicts/issues with the library.

I can change the background for yaml viewer to grayish for the time being. That will be quick.

#2: I agree. Any thoughts on having a separate markdown file for that purpose?

#3: I know. But if we want include more information, there's no way around that, unless we remove the property or redact the value which is probably not a good idea.

@daniel-de-vera
Copy link
Contributor

#1: The current design for yaml editor is temporary. I plan to use a different off-the-shelf component for displaying yaml afterwards. I had actually done that in one of the pull requests. But had to scrap that because the version update for webpack led to some conflicts/issues with the library.

I can change the background for yaml viewer to grayish for the time being. That will be quick.

It's good to know that the change is part of the roadmap.

#2: I agree. Any thoughts on having a separate markdown file for that purpose?

I guess we could make the description of our plugins richer and store them in separate files, yes.

#3: I know. But if we want include more information, there's no way around that, unless we remove the property or redact the value which is probably not a good idea.

I think for an end state, we can have a much richer UI by extracting info from the plugin spec.
For example, we could:

  • Automatically display all inputs and outputs (without counting on the user defining this as part of the description)
  • Display details about the create and delete workflows, showing for each step the input/outputs, the scripts used, the dependencies, etc.
  • Display information about the runner

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