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

ref: Replace statik with Go embed #109

Merged
merged 1 commit into from
Mar 1, 2021
Merged

ref: Replace statik with Go embed #109

merged 1 commit into from
Mar 1, 2021

Conversation

ghost
Copy link

@ghost ghost commented Feb 26, 2021

Closes #107
Signed-off-by: david-doit-intl david@doit-intl.com

@ghost ghost requested a review from stepanstipl February 26, 2021 12:24
@ghost ghost force-pushed the feature/goEmbed branch 7 times, most recently from 811981a to 426c87d Compare February 26, 2021 15:02
@ghost ghost changed the title WIP: feat: Add go embed removed static upgrade to go1.16 feat: Add go embed removed static upgrade to go1.16 Feb 26, 2021
@ghost ghost linked an issue Feb 26, 2021 that may be closed by this pull request
@ghost ghost assigned stepanstipl and ghost Feb 26, 2021
@ghost ghost force-pushed the feature/goEmbed branch 2 times, most recently from d0c077d to 5c16cf2 Compare February 26, 2021 15:45
@stepanstipl stepanstipl changed the title feat: Add go embed removed static upgrade to go1.16 ref: Add go embed removed static upgrade to go1.16 Feb 28, 2021
@stepanstipl
Copy link
Contributor

stepanstipl commented Feb 28, 2021

Ok, I like getting rid of statik, looks simpler and certainly nicer to use Go-native feature now that it's available 🎉 🎉 , thanks @david-doit-intl!

2 minor things:

  • Can we split this in 2 PRs - one for Go 1.16 upgrade (or at least commits, but changelog is generated based on PRs), one for getting rid of statik
  • Can we keep the rules in top-level directory? I would a) prefer to have those there and b) not to change the path. Or is this a limitation of the embed?

@stepanstipl
Copy link
Contributor

Just noticed there's already #101 by bot, perhaps we can move the whole Go 1.16 upgrade there.

@ghost
Copy link
Author

ghost commented Mar 1, 2021

@stepanstipl yeah lets get 1.16 in first with the bot, then I can merge :). Will try to move rules back to top level, had some issues but will try again.

@ghost ghost changed the title ref: Add go embed removed static upgrade to go1.16 ref: Add go embed removed static Mar 1, 2021
@ghost ghost force-pushed the feature/goEmbed branch 2 times, most recently from 29a3a90 to 877e3d7 Compare March 1, 2021 09:09
@ghost ghost changed the title ref: Add go embed removed static feat: Add go embed removed static Mar 1, 2021
@ghost ghost unassigned stepanstipl Mar 1, 2021
@ghost ghost force-pushed the feature/goEmbed branch 2 times, most recently from d7f3833 to 18075e3 Compare March 1, 2021 11:21
@ghost
Copy link
Author

ghost commented Mar 1, 2021

@stepanstipl updated to isolate rules to its own package pretty happy now.

@@ -64,7 +65,12 @@ func TestEvalRules(t *testing.T) {
manifests = append(manifests, manifest)
}

judge, err := NewRegoJudge(&RegoOpts{})
loadedRules, err := rules.FetchRegoRules()
Copy link
Author

Choose a reason for hiding this comment

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

We should make some mock rules in a later PR

@ghost ghost force-pushed the feature/goEmbed branch 2 times, most recently from fb1789b to b87270e Compare March 1, 2021 16:37
Signed-off-by: david-doit-intl <david@doit-intl.com>
@stepanstipl stepanstipl changed the title feat: Add go embed removed static ref: Replace statik with Go embed Mar 1, 2021
Copy link
Contributor

@stepanstipl stepanstipl left a comment

Choose a reason for hiding this comment

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

Very nice 👍 , I really like this last iteration 🚀

@stepanstipl stepanstipl merged commit 12652c7 into master Mar 1, 2021
@stepanstipl stepanstipl deleted the feature/goEmbed branch March 1, 2021 17:11
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.

Use go embed instead of static
1 participant