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.

Sign up for GitHub

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

Jump to bottom

Fix version flag output #1171

Merged
merged 2 commits into from
Apr 26, 2024
Merged

Fix version flag output #1171

merged 2 commits into from
Apr 26, 2024

Conversation

haoming29
Copy link
Contributor

Fixes #1169

@haoming29 haoming29 requested a review from turetske April 26, 2024 14:57
@haoming29 haoming29 added this to the v7.8.0 milestone Apr 26, 2024
@haoming29 haoming29 added bug Something isn't working critical High priority for next release labels Apr 26, 2024
@haoming29
Copy link
Contributor Author

The macOS test failed at installing dependencies, which I have no clue but shouldn't be related to this PR.

turetske
turetske requested changes Apr 26, 2024
View reviewed changes
Copy link
Collaborator

@turetske turetske left a comment

Choose a reason for hiding this comment

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

Is the version_test.sh script being run anywhere? I'd suggest adding it to the GitHub actions test action.

@haoming29
Copy link
Contributor Author

Is the version_test.sh script being run anywhere? I'd suggest adding it to the GitHub actions test action.

Good call! I missed that part

@haoming29 haoming29 force-pushed the fix-version-out branch 2 times, most recently from 3f98a74 to 0e791cf Compare April 26, 2024 15:57
@haoming29
Copy link
Contributor Author

haoming29 commented Apr 26, 2024 •
edited
Loading

Since Emma is out for the rest of the day and we want to release the patch ASAP, assigned @jhiemstrawisc to take another look

jhiemstrawisc
jhiemstrawisc requested changes Apr 26, 2024
View reviewed changes
Copy link
Member

@jhiemstrawisc jhiemstrawisc left a comment

Choose a reason for hiding this comment

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

Minor edits. The guts all look good to me.

cmd/main_test.go Outdated Show resolved Hide resolved
cmd/main_test.go Outdated Show resolved Hide resolved
github_scripts/version_test.sh Outdated Show resolved Hide resolved
@haoming29 haoming29 requested a review from jhiemstrawisc April 26, 2024 19:02
jhiemstrawisc
jhiemstrawisc approved these changes Apr 26, 2024
View reviewed changes
Copy link
Member

@jhiemstrawisc jhiemstrawisc left a comment

Choose a reason for hiding this comment

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

LGTM

@jhiemstrawisc
Copy link
Member

Since Emma is out and this is urgent, I'm going to merge.

@jhiemstrawisc jhiemstrawisc merged commit 2335dcf into PelicanPlatform:main Apr 26, 2024
17 of 19 checks passed
@haoming29 haoming29 deleted the fix-version-out branch April 26, 2024 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhiemstrawisc jhiemstrawisc jhiemstrawisc approved these changes

@turetske turetske Awaiting requested review from turetske

Assignees
No one assigned
Labels
bug Something isn't working critical High priority for next release
Projects
None yet
Milestone
v7.8.0
Development

Successfully merging this pull request may close these issues.

Pelican --version prints to stderr instead of stdout
3 participants
@haoming29 @jhiemstrawisc @turetske