Skip to content

Conversation

@dluftspring
Copy link

Description

👋 Just throwing this up here for review as I think I ran into a single query error using this action. My read is that when you pass a single query trying to split on the ; causes an empty string to be sent to snowflake which then throws an error on the action. If you'd prefer to just document this behaviour as opposed to introducing new code that makes sense but I thought I would propose these changes as a suggestion

@talboren talboren requested a review from dapollak July 10, 2022 15:12
Copy link
Contributor

@talboren talboren left a comment

Choose a reason for hiding this comment

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

See some comments 🙏🏼

main.py Outdated
load_dotenv() # only on local run
print(os.environ)
queries_list = os.environ['INPUT_QUERIES'].split(';')
queries_list = map(str.strip, os.environ['INPUT_QUERIES'].split(';'))
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this required?

Copy link
Author

Choose a reason for hiding this comment

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

Just for safety to get rid of unnecessary whitespace

main.py Outdated
Comment on lines 34 to 39
if query:
query_result = con.query(query)
query_results.append(query_result)
print("### Running query ###")
print(f"[!] Query id - {query_result.query_id}")
print(f"[!] Running query ### - {query}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps just filter out any None or empty query from queries_list instead of duplicating this if twice?

Copy link
Author

Choose a reason for hiding this comment

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

So you'd find it cleaner if we used filter when generating the queries_list?

Copy link
Contributor

@dapollak dapollak left a comment

Choose a reason for hiding this comment

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

Copy link

@orca-security-us orca-security-us bot left a comment

Choose a reason for hiding this comment

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

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Infrastructure as Code high 0   medium 0   low 0   info 0 View in Orca
Passed Passed SAST high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Vulnerabilities high 0   medium 0   low 0   info 0 View in Orca

@dluftspring
Copy link
Author

👋 @dapollak @talboren better late than never here. Not a huge change but I've added the requested feedback. Think this somehow got lost in GitHub notifications for three years!

@talboren
Copy link
Contributor

talboren commented Jun 11, 2025

👋 @dapollak @talboren better late than never here. Not a huge change but I've added the requested feedback. Think this somehow got lost in GitHub notifications for three years!

@dluftspring sorry to let you down, but we're not maintaining this one anymore since we're not at @anecdotes-ai anymore. You'll need someone from the team to review it -- I'll try to ping someone

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