Skip to content

Conversation

@ericloyd
Copy link
Contributor

PR adds --stdin option to allow text to be piped in, overriding the --text option. This is per #62

@ChaoticWeg ChaoticWeg added the enhancement New feature or request label May 21, 2025
@ChaoticWeg ChaoticWeg self-requested a review May 21, 2025 13:34
Copy link
Collaborator

@ChaoticWeg ChaoticWeg left a comment

Choose a reason for hiding this comment

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

LGTM

@ChaoticWeg
Copy link
Collaborator

Looks like tests failed, I'm not familiar with the framework/setup used for testing here, so will defer to @fieu

@ericloyd
Copy link
Contributor Author

I'm new to bats so I may have built the test wrong. You can test manually from the command line:

echo "this text will be used" | ./discord.sh --username "username test" --text="this text is ignored" --stdin

@ericloyd
Copy link
Contributor Author

This is the bats test file that worked for me, without all the other tests in it.

#!/usr/bin/env bats

load pre

# Test that text can be sent via pipe
@test "text will be ignored, gathered instead from stdin (pip)" {
    run bats_pipe echo "this text will be used" \| discord.sh --username "username test" --text="this text is ignored" --stdin
    [ "$status" -eq 0 ]
}

@ericloyd
Copy link
Contributor Author

Nevermind. I see I forgot a closing } in the 01 tests file. :-(

@ChaoticWeg
Copy link
Collaborator

Nevermind. I see I forgot a closing } in the 01 tests file. :-(

Yup, only just now noticed it myself, lol. That's what I get for reviewing code at 8 AM.

@ChaoticWeg ChaoticWeg self-requested a review May 21, 2025 13:51
@ChaoticWeg
Copy link
Collaborator

No obvious errors to me, test still fails. Deferring to fieu

@ericloyd
Copy link
Contributor Author

Weird. When I pull that single test out separately, it works. I didn't want to run them all on my testing, but I'll run through them to see if I can figure it out.

@ericloyd
Copy link
Contributor Author

84 tests, 0 failures

@T0biii
Copy link
Contributor

T0biii commented May 21, 2025

discord.sh cant be found for the pipe

✗ text will be ignored, gathered instead from stdin (pip)
   (in test file tests/01.text.bats, line 69)
     `[ "$status" -eq 0 ]' failed
   Last output:
   /usr/local/lib/bats-core/test_functions.bash: line 276: discord.sh: command not 

84 tests, 1 failure

Co-authored-by: Tobias <5702338+T0biii@users.noreply.github.com>
@ericloyd
Copy link
Contributor Author

Thanks for that catch. I had a symlink in my path for discord.sh that was finding the file in my local repo. Good catch!

@T0biii
Copy link
Contributor

T0biii commented May 21, 2025

mh ci still failing maybe the secret for the weebhook is broken?
when i setup in my fork the secret and run the pipline it works fine:
https://github.com/T0biii/discord.sh/actions/runs/15165388096/job/42641721325

image

image

@ericloyd
Copy link
Contributor Author

I'm not sure what to do here so I'll leave it hanging in the ether until someone smarter than me makes decisions. :-)

@ChaoticWeg
Copy link
Collaborator

That'll be @fieu with the knowledge on how the actions and secrets are set up here, the code looks fine to me, so up to him whether to merge with failing tests, whether to fix, whatever we wanna do there.

Thanks for the PR @ericloyd !

@ericloyd
Copy link
Contributor Author

@fieu Any luck with you having a chance to check out why the automated tests are failing? Works from command line and the PR is pretty simple. Would love to see this go through.

@ericloyd
Copy link
Contributor Author

@fieu Sheldon, it's been two months for this relatively simple PR. Any chance you can take a look and push it through? I'm confident that the tests are failing because of the build environment, not the code.

@fieu
Copy link
Owner

fieu commented Oct 27, 2025

Hey @ericloyd,
I'm really sorry this has taken so long to get merged. I've been dealing with some real life stuff and haven't been able to allocate much time to the project, but I now have more time to work on it.

When I first saw the GitHub Action workflow fail, I was too preoccupied with other things to dig into why it was failing. It was puzzling considering your change was very minor, but I just didn't have the bandwidth to investigate at the time.

After some digging (albeit 5 months later), I found out that even when approving a pull request to run the action workflow, the action won't have access to the repository secrets (which we use for the $DISCORD_WEBHOOK environment variable). This wasn't apparent at the time and I didn't have the time to look into it. I've actually never dealt with pull requests and actions before. We used to be on Travis CI, so I've learned a lot since then.

I noticed @T0biii mentioned the secrets might not be working properly in the action, and he was spot on. GitHub does this (as they should) from a security perspective to avoid leaks of secrets.

Overall, after doing some testing locally and verifying the pipeline passed, it looks good to me. Again, I apologize for the huge delay, and thank you for your patience and contribution!

@fieu fieu merged commit 76cc2ca into fieu:master Oct 27, 2025
1 check failed
@ericloyd
Copy link
Contributor Author

We're all good. I know how RL can be. Thanks?

@fieu
Copy link
Owner

fieu commented Oct 27, 2025

Tagged v2.0.1, thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants