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

Add modular output #130

Merged
merged 3 commits into from Jul 10, 2020
Merged

Add modular output #130

merged 3 commits into from Jul 10, 2020

Conversation

0legovich
Copy link
Contributor

@0legovich 0legovich commented Jun 2, 2020

Added the possibility of optional output of noisy information
Closes #78

@Arkweid
Copy link
Collaborator

@Arkweid Arkweid commented Jun 3, 2020

@0legovich Awesome! Will look at soon.

@0legovich
Copy link
Contributor Author

@0legovich 0legovich commented Jun 3, 2020

@Arkweid, what do you think about:

  1. Explicitly specify the absence of output log: output: false (added in the current implementation).
  2. In the current implementation, if there is no output log, information about the running command/script will still be displayed. For example rubocop (SKIP. NO FILES FOR INSPECTION) or EXECUTE > rubocop. Maybe it makes sense to add another output value - stages?
  3. Is it correct not to show any message if the runner finished with a non-zero code status?

@Arkweid
Copy link
Collaborator

@Arkweid Arkweid commented Jun 4, 2020

@0legovich

Explicitly specify the absence of output log: output: false (added in the current implementation).

It not very useful for me, but maybe others will find it as a good option in CI scenarios. So, okay.

In the current implementation, if there is no output log, information about the running command/script will still be displayed. For example rubocop (SKIP. NO FILES FOR INSPECTION) or EXECUTE > rubocop. Maybe it makes sense to add another output value - stages?

Don't see a lot of sense for that. The best option is to associate this output with failures and success runners.

Is it correct not to show any message if the runner finished with a non-zero code status?

If user set only summary - so not. Lets focus on most userful scenario. Most of devs want this:

output:
  - failures #(output from runners with exit code nonzero)
  - summary #(summary block)

They want to debug problems by log if something down and see a summary(tasks were completed).

@0legovich
Copy link
Contributor Author

@0legovich 0legovich commented Jun 4, 2020

@Arkweid

They want to debug problems by log if something down and see a summary(tasks were completed).

Maybe then it makes sense to go another way, for example:

skip_output:
  - meta
  - success

In this case, the user will always see the runners result regardless of whether there were failures or not. However, if there were failures, they will be output to log.

So I suggest removing the option to skip the output of summary and failures in the log.

@0legovich 0legovich changed the title Add modular output WIP: Add modular output Jun 4, 2020
@Arkweid
Copy link
Collaborator

@Arkweid Arkweid commented Jun 4, 2020

@0legovich looks good for me.

@0legovich
Copy link
Contributor Author

@0legovich 0legovich commented Jun 4, 2020

@Arkweid thank you for your quick answers.

If we associate "EXECUTE > command_or_script_name" output with success or failure runners, then the behavior of the application will change.

Before:

EXECUTE > command_or_script_name
<duration of the command>
Failure log

After:

<duration of the command>
EXECUTE > command_or_script_name
Failure log

Is this critical for this app? Any ideas?

@Arkweid
Copy link
Collaborator

@Arkweid Arkweid commented Jun 5, 2020

@0legovich
I don't think it's critical. It's acceptable behavior.
Probably there needs some spinner element to inform users about lefthook still works. But it out of scope this task, so your choice.

@0legovich 0legovich force-pushed the modular_output branch 3 times, most recently from c3321bc to 9bf7d9f Compare Jun 8, 2020
@0legovich
Copy link
Contributor Author

@0legovich 0legovich commented Jun 8, 2020

@Arkweid Hi. The idea with the spinner seemed good to me. Please check the code.

@0legovich 0legovich changed the title WIP: Add modular output Add modular output Jun 8, 2020
@Arkweid
Copy link
Collaborator

@Arkweid Arkweid commented Jun 9, 2020

@0legovich take a look at run_windows.go. It a file for windows ecosystem compile. Copy existed code into.
run and run_windows mostly the same. But Windows haven't pty interface.
We cannot easily synchronize the output, but it okay for windows :)

@0legovich
Copy link
Contributor Author

@0legovich 0legovich commented Jun 10, 2020

@Arkweid, thank you. I added this functionality for the Windows ecosystem.

@Arkweid
Copy link
Collaborator

@Arkweid Arkweid commented Jun 10, 2020

Will test soon.

@Arkweid
Copy link
Collaborator

@Arkweid Arkweid commented Jun 17, 2020

@0legovich I have tested PR for discourse example(my OS is Ubuntu 18) and got this output:

Lefthook v0.7.2
RUNNING HOOKS GROUP: pre-commit

  EXECUTE > eslint-assets
 read /dev/ptmx: input/output error

  EXECUTE > eslint-assets-tests
 read /dev/ptmx: input/output error

  EXECUTE > eslint-plugins-assets
 read /dev/ptmx: input/output error

  EXECUTE > eslint-plugins-test
 read /dev/ptmx: input/output error

  EXECUTE > eslint-test
 read /dev/ptmx: input/output error

  EXECUTE > prettier
 read /dev/ptmx: input/output error

SUMMARY: (done in 15.13 seconds)
🥊  eslint-assets
🥊  eslint-assets-tests
🥊  eslint-plugins-assets
🥊  eslint-plugins-test
🥊  eslint-test
🥊  prettier

Probably this one is root of problem:
https://github.com/Arkweid/lefthook/pull/130/files#diff-e540b5efa4ce165399814e0fc4016fa4R267

This is how to reproduce.

  1. Add Dockerfile in repo
FROM ruby:2.6.6-slim-buster

# Common dependencies
RUN apt-get update -qq \
  && DEBIAN_FRONTEND=noninteractive apt-get install -yq --no-install-recommends \
    build-essential \
    gnupg2 \
    curl \
    git \
  && apt-get clean \
  && rm -rf /var/cache/apt/archives/* \
  && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* \
  && truncate -s 0 /var/log/*log

# Add NodeJS to sources list
RUN curl -sL https://deb.nodesource.com/setup_12.x | bash -

# Add Yarn to the sources list
RUN curl -sS https://dl.yarnpkg.com/debian/pubkey.gpg | apt-key add - \
  && echo 'deb http://dl.yarnpkg.com/debian/ stable main' > /etc/apt/sources.list.d/yarn.list

RUN apt-get update -qq && DEBIAN_FRONTEND=noninteractive apt-get -yq dist-upgrade && \
  DEBIAN_FRONTEND=noninteractive apt-get install -yq --no-install-recommends \
    libpq-dev \
    nodejs \
    yarn

RUN git clone https://github.com/discourse/discourse.git --depth=1

WORKDIR /discourse

RUN gem update --system && \
    gem install bundler

RUN bundle install
RUN yarn install

COPY lefthook .
CMD ["bash", "-xc", "./lefthook run lints"]
  1. Build lefthook
go build
  1. Build dockerfile
sudo docker build -t discourse-lefthook-test .
  1. Execute dockerfile
docker run --rm -ti discourse-lefthook-test

PS. You could ensure the existed lefthook 0.7.2 version works fine with this command:

docker run --rm -ti discourse-lefthook-test yarn lefthook run lints

@0legovich
Copy link
Contributor Author

@0legovich 0legovich commented Jun 22, 2020

@Arkweid Thank you for your detailed comment.
I fixed this behavior.

@Arkweid
Copy link
Collaborator

@Arkweid Arkweid commented Jun 24, 2020

Will look at soon.

@Arkweid
Copy link
Collaborator

@Arkweid Arkweid commented Jul 10, 2020

Sorry for so long @0legovich
Looks fine! Merged :)

@Arkweid Arkweid merged commit 7de5f82 into evilmartians:master Jul 10, 2020
1 check passed
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.

2 participants