Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

migrate to go module #50

Merged
merged 5 commits into from
Dec 6, 2019
Merged

Conversation

honnix
Copy link
Member

@honnix honnix commented Nov 29, 2019

No description provided.

@kumare3
Copy link
Contributor

kumare3 commented Nov 29, 2019

Some tests are failing? But this looks awesome.

EngHabu
EngHabu previously approved these changes Nov 29, 2019
Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

Thank you a ton for getting this done!

go.mod Show resolved Hide resolved
@honnix
Copy link
Member Author

honnix commented Nov 30, 2019

This and flyteidl don't work and there was weird issue that I haven't solved.

@honnix
Copy link
Member Author

honnix commented Nov 30, 2019

golang/go@f851253 might be related.

@honnix
Copy link
Member Author

honnix commented Dec 4, 2019

I believe the problem is here https://github.com/lyft/flytestdlib/blob/master/cli/pflags/api/generator.go#L304 This doesn't support module. I'm looking into using packages instead golang/go#26433

@codecov-io
Copy link

codecov-io commented Dec 4, 2019

Codecov Report

Merging #50 into master will increase coverage by 0.01%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
+ Coverage   71.95%   71.96%   +0.01%     
==========================================
  Files          56       56              
  Lines        2232     2240       +8     
==========================================
+ Hits         1606     1612       +6     
- Misses        503      504       +1     
- Partials      123      124       +1
Impacted Files Coverage Δ
cli/pflags/api/generator.go 73.7% <77.77%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f921467...2b46eff. Read the comment docs.

@honnix
Copy link
Member Author

honnix commented Dec 4, 2019

Well it works!

@honnix
Copy link
Member Author

honnix commented Dec 4, 2019

@EngHabu PTAL again. We will need to release a new version of pflags cli.

@honnix
Copy link
Member Author

honnix commented Dec 4, 2019

Another thing to mention is, some test cases won't pass on Mac due to "too many opened files" in some temp folder.

@EngHabu
Copy link
Contributor

EngHabu commented Dec 6, 2019

Awesome!! Love it! Thank you for investigating and fixing the goimports issue
Yeah as soon as we merge this to master, please follow these steps: https://github.com/lyft/flytestdlib#releases to package and release a new version of pflags

@EngHabu EngHabu merged commit 1d00a51 into flyteorg:master Dec 6, 2019
@honnix honnix deleted the go-module-migration branch December 9, 2019 18:53
wild-endeavor added a commit to flyteorg/boilerplate that referenced this pull request Jan 23, 2020
This updates the boilerplate to use go modules instead of dep and follows the changes that have already been made:

* flyteorg/datacatalog#21
* flyteorg/flytepropeller#38
* flyteorg/flyteadmin#35
* flyteorg/flyteplugins#36
* flyteorg/flytestdlib#50
* flyteorg/flyteidl#27 (depends on flyteorg/flytestdlib#50 to be merged and released)
eapolinario pushed a commit to eapolinario/flyte that referenced this pull request Aug 21, 2023
This updates the boilerplate to use go modules instead of dep and follows the changes that have already been made:

* flyteorg/datacatalog#21
* flyteorg/flytepropeller#38
* flyteorg/flyteadmin#35
* flyteorg/flyteplugins#36
* flyteorg/flytestdlib#50
* flyteorg/flyteidl#27 (depends on flyteorg/flytestdlib#50 to be merged and released)
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* migrate to go module

* tidy

* do not force

* try packages

* extract into a method
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants