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

fix: replace deprecated bouk/staticfiles with native Go embed. fixes #11654 #11707

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

sonbui00
Copy link
Member

Fixes #11654

Motivation

  • bouk/staticfiles is deprecated
  • Sometime server/static/files.go is added to git commit unexpectedly

Modifications

  • remove bouk/staticfiles, use standard lib

Verification

  • make test
  • manual check

@sonbui00 sonbui00 force-pushed the fix-11654 branch 2 times, most recently from 542605d to a2dc0b1 Compare August 29, 2023 13:31
@agilgur5 agilgur5 added type/dependencies PRs and issues specific to updating dependencies go Pull requests that update Go dependencies labels Aug 29, 2023
@sonbui00 sonbui00 marked this pull request as ready for review August 30, 2023 02:47
@stale
Copy link

stale bot commented Sep 17, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the problem/stale This has not had a response in some time label Sep 17, 2023
@sonbui00
Copy link
Member Author

@agilgur5 Should I do anything to go forward for this?

@agilgur5 agilgur5 removed the problem/stale This has not had a response in some time label Sep 20, 2023
@agilgur5
Copy link

agilgur5 commented Sep 20, 2023

Usually the process is that someone does initial reviews and if it's not an approver, we wait for an approver to then approve and merge.

Workflows doesn't have many active approvers so there's occasionally a backlog as there is now. I also have some PRs that have been open for 3+ weeks 😕 That is probably the longest wait I've seen in the past few months though, fwiw (I've only been actively contributing the past few, so can't say beyond that).

I had initially deferred review to someone who may know embed etc better (I have used it exactly once as part of a CLI, not a server), but Workflows might not actually have any contributors familiar with it, so maybe I should do first review.

@agilgur5
Copy link

agilgur5 commented Sep 20, 2023

For context, I am currently a Member (though may become a Reviewer at the next membership meeting). Lead > Approver > Reviewer > Member. You can see the Argo governance docs here.
You are also welcome to request Membership if you're interested 😉 (we can probably find a sponsor as well; sponsors must be approvers+, though support from others in the community like me helps!)

Copy link
Member

@isubasinghe isubasinghe left a comment

Choose a reason for hiding this comment

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

I am happy with this, looks sensible, the one suggestion I would like to state is that perhaps some generation of the strings used for ByteReedSeeker could be done? Currently only a single "hello world" string is used in all the test cases.

server/static/static.go Show resolved Hide resolved
@isubasinghe isubasinghe requested a review from agilgur5 January 14, 2024 09:38
@agilgur5 agilgur5 changed the title fix: bouk/staticfiles is deprecated #11654 fix: replace deprecated bouk/staticfiles with native Go embed. fixes #11654 Feb 21, 2024
@juliev0
Copy link
Contributor

juliev0 commented Jun 28, 2024

@sonbui00 sorry this ended up being delayed. I see @isubasinghe approved this awhile back. @isubasinghe is now an Approver and can merge. Do you want to fix the merge conflicts and get this merged still?

@sonbui00
Copy link
Member Author

sonbui00 commented Jun 29, 2024

@juliev0 I'll check it

@juliev0
Copy link
Contributor

juliev0 commented Jul 3, 2024

let me know when you've fixed the codegen and lint issues and we can merge this

@sonbui00
Copy link
Member Author

sonbui00 commented Jul 4, 2024

@juliev0 codegen and lint issues have been fixed. help me review

@juliev0
Copy link
Contributor

juliev0 commented Jul 4, 2024

Sorry @isubasinghe - it looks like your original review has been removed with the fixes @sonbui00 made . Can you re-review/merge when you have time?

@agilgur5 agilgur5 removed their assignment Nov 15, 2024
@isubasinghe
Copy link
Member

@sonbui00 Could you please fix the merge conflicts? I will merge this in ASAP, sorry about forgetting to re-review this, just have a million things to do atm.

util/io/bytereadseeker.go Outdated Show resolved Hide resolved
util/io/subdirfs.go Outdated Show resolved Hide resolved
@sonbui00 sonbui00 force-pushed the fix-11654 branch 2 times, most recently from 30a93a3 to 766de34 Compare December 2, 2024 16:55
ui/dist/app/gitkeep Outdated Show resolved Hide resolved
ui/.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Copy link
Member

@Joibel Joibel left a comment

Choose a reason for hiding this comment

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

Thanks so much for all your diligence on working through this one.

@Joibel Joibel merged commit 10aaf3e into argoproj:main Dec 6, 2024
28 checks passed
This was referenced Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go dependencies type/dependencies PRs and issues specific to updating dependencies type/tech-debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecated - Static files embed library
5 participants