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

proposal: Ignore vendor directory #776

Merged
merged 1 commit into from
May 9, 2023
Merged

proposal: Ignore vendor directory #776

merged 1 commit into from
May 9, 2023

Conversation

candiduslynx
Copy link
Contributor

@candiduslynx candiduslynx commented Apr 18, 2023

Description

Please explain the changes you made here.

Checklist

  • Code compiles correctly
  • Run make fmt to fix inconsistent formats
  • Run make lint to get lint errors and fix all of them
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary

@candiduslynx candiduslynx marked this pull request as ready for review April 18, 2023 06:12
@candiduslynx candiduslynx requested a review from a team as a code owner April 18, 2023 06:12
@candiduslynx
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@candiduslynx
Copy link
Contributor Author

recheck

@sfc-gh-dszmolka
Copy link
Contributor

#729

@candiduslynx
Copy link
Contributor Author

@sfc-gh-dszmolka any chance of review/running the workflows?

@sfc-gh-dszmolka
Copy link
Contributor

PRs posted in this repo are regularly reviewed by the driver team, cannot promise any estimated timeline though. Thank you for bearing with us !

@sfc-gh-igarish
Copy link
Collaborator

could you please add more details about this PR?

@candiduslynx
Copy link
Contributor Author

Hi @sfc-gh-igarish!

The reasoning could be linked to #729.

Basically, it's not required to save the vendor inside the repo unless the codebase has some manual updates (replaces, etc).

Additionally, having this lot of files results in huge updates when it should've been only a small commit.
Example: #769

@zeroshade
Copy link
Contributor

I agree with this personally! It will make updates of dependencies significantly easier going forward if we can get rid of the vendor directory entirely.

@candiduslynx
Copy link
Contributor Author

@sfc-gh-igarish is there a chance to get this reviewed/merged?
One of the reasons for this proposal can be seen at the moment (conflicts over simple vendor changes, that should've been scoped to go.mod & go.sum files only)
image

@candiduslynx
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@candiduslynx
Copy link
Contributor Author

Hi @sfc-gh-igarish! Any chance on reviewing this?

@zeroshade
Copy link
Contributor

@candiduslynx they will probably ask you to rebase this back up-to-date. So can you pre-emptively do that and let's hope we can get their attention 😄

@zeroshade zeroshade mentioned this pull request May 8, 2023
4 tasks
Copy link
Collaborator

@sfc-gh-igarish sfc-gh-igarish left a comment

Choose a reason for hiding this comment

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

LGTM

@candiduslynx
Copy link
Contributor Author

@sfc-gh-igarish thanks for the review!
Do I need to do something special to make CLA pass?

@sfc-gh-igarish
Copy link
Collaborator

@sfc-gh-igarish thanks for the review! Do I need to do something special to make CLA pass?

Let's wait. I will let you know.

@sfc-gh-igarish sfc-gh-igarish merged commit 40d48c8 into snowflakedb:master May 9, 2023
22 of 42 checks passed
@candiduslynx candiduslynx deleted the patch-1 branch May 10, 2023 09:05
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.

None yet

4 participants