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

Add a script for automatically updating scheme #2

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

wzshiming
Copy link
Member

@wzshiming wzshiming commented Feb 15, 2024

Currently manually maintained dependencies may be lost, so a new script has been added to keep them in sync

xref #3

@wzshiming wzshiming force-pushed the ci/scheme branch 3 times, most recently from 2bf2941 to a80469b Compare February 15, 2024 10:17
@siyuanfoundation
Copy link
Contributor

This is awesome! Thank you @wzshiming for working on this!

@wzshiming wzshiming force-pushed the ci/scheme branch 6 times, most recently from 1282325 to 5d0da2f Compare February 18, 2024 05:06
serializer "k8s.io/apimachinery/pkg/runtime/serializer"
)

var Registry = registered.NewOrDie(os.Getenv("KUBE_API_VERSIONS"))
Copy link
Member Author

@wzshiming wzshiming Feb 18, 2024

Choose a reason for hiding this comment

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

This is not used in the current project and has been removed in the latest k8s version, maybe we should remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, let's remove it.

schedulingv1alpha1 "k8s.io/api/scheduling/v1alpha1"

settingsv1alpha1 "k8s.io/api/settings/v1alpha1"
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we somehow support api's that have been removed?

#10

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is fine to not support apis that have been removed. But we should

  • Build and publish releases for all recent kubernetes versions (1.6+)
  • Can you add instructions about how to build the tool for an specific version of k8s in README using the script? It can be a follow up PR.

Copy link
Member Author

@wzshiming wzshiming Feb 29, 2024

Choose a reason for hiding this comment

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

Build and publish releases for all recent kubernetes versions (1.6+)

I think it would be possible to use a script to copy the deleted apiversion to the current repository, similar to what this PR currently does. #10

WDYT?

Can you add instructions about how to build the tool for an specific version of k8s in README using the script? It can be a follow up PR.

Of course, I've moved the initialisation of scheme to main, which allows different versions to co-exist, and I'll try to improve the documentation later.

Copy link
Contributor

Choose a reason for hiding this comment

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

while it would be possible to use a script to copy the deleted apiversion to the current repository, we still need to make decisions about how far back we want to support. We can start a discussion in the other issue.

pkg/encoding/scheme.sh Outdated Show resolved Hide resolved
pkg/encoding/scheme.sh Outdated Show resolved Hide resolved
@wzshiming wzshiming force-pushed the ci/scheme branch 2 times, most recently from ed7a0db to 74bc7c4 Compare February 29, 2024 03:14
@wzshiming
Copy link
Member Author

Done

@wzshiming wzshiming force-pushed the ci/scheme branch 5 times, most recently from 87af109 to aa77ea0 Compare February 29, 2024 03:37
cmd/init_test.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Member Author

@wzshiming wzshiming Mar 1, 2024

Choose a reason for hiding this comment

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

Sorry I mistook it for a comment on a Makefile change

By default, Makefile targets are "file targets" - they are used to build files from other files. Make assumes its target is a file, and this makes writing Makefiles relatively easy:

foo: bar
  create_one_from_the_other foo bar

However, sometimes you want your Makefile to run commands that do not represent physical files in the file system. Good examples for this are the common targets "clean" and "test". Chances are this isn't the case, but you may potentially have a file named clean in your main directory. In such a case Make will be confused because by default the clean target would be associated with this file and Make will only run it when the file doesn't appear to be up-to-date with regards to its dependencies.

These special targets are called phony and you can explicitly tell Make they're not associated with files, e.g.:

.PHONY: clean
clean:
  rm -rf *.o

Now make clean will run as expected even if you do have a file named clean.

In terms of Make, a phony target is simply a target that is always out-of-date, so whenever you ask make <phony_target>, it will run, independent from the state of the file system.

See also: GNU make manual: Phony Targets

Copy link
Member Author

@wzshiming wzshiming Mar 4, 2024

Choose a reason for hiding this comment

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

I moved the registration of scheme to a separate package pkg/scheme, if I hadn't introduced it then scheme wouldn't have existed in this test and it wouldn't have passed the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

you can move

func init() {
	AddToScheme(scheme.Scheme)
}

to pkg/encoding/encoding.go, and the init file would not be needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a reason for this.

Imagine a scenario where I introduce this package in another project, but due to the differences in apiversion of the different versions of k8s used this package cannot be used, because some paths may not exist.

If I do this, the other project can take care of registering the scheme itself.

Copy link
Member Author

@wzshiming wzshiming Mar 5, 2024

Choose a reason for hiding this comment

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

I filed an issue #10 that I planned to implement by writing a script to copy the deleted scheme to "github.com/etcd-io/auger/pkg/old_scheme"

If the caller doesn't care about the k8s version, they can just use

import (
	_ "github.com/etcd-io/auger/pkg/scheme"
	_ "github.com/etcd-io/auger/pkg/old_scheme"
)

otherwise regenerate scheme in their own repo using the two scripts we provided

import (
	_ "path/to/pkg/scheme"
	_ "path/to/pkg/old_scheme"
)

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, should the schemes be passed in as arguments toAddToScheme?
When doing

import (
	_ "path/to/pkg/scheme"
	_ "path/to/pkg/old_scheme"
)

you need to consider duplicates and conflicts.

cmd/init_test.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

you can move

func init() {
	AddToScheme(scheme.Scheme)
}

to pkg/encoding/encoding.go, and the init file would not be needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

please add init() function in the encoding.go instead of a new init file. init files are more like a python pattern instead of go.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am planning to refactor encoding.go to pass these global variables as arguments, at which point the file will be removed.

The core goal is to be able to import this package directly from other projects.

Copy link
Contributor

@siyuanfoundation siyuanfoundation Mar 8, 2024

Choose a reason for hiding this comment

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

I like the idea of passing arguments. To import this package directly into other projects, I think that can be better achieved by adding functions like UpdateScheme and MergeSchemes if multiple schemes were to be used.
Or like you said, refactor the code to have Codecs passed in everywhere instead of having a global variable.

The init files everywhere as this PR goes, are not necessary and looks weird.

@wzshiming wzshiming force-pushed the ci/scheme branch 2 times, most recently from c023211 to 69179b5 Compare March 5, 2024 03:05
@wzshiming
Copy link
Member Author

This PR requires a member to approve the execution of the workflow

/cc @siyuanfoundation
/cc @jmhbnz
/cc @wenjiaswe

@jmhbnz
Copy link
Member

jmhbnz commented Mar 5, 2024

/ok-to-test

@wzshiming
Copy link
Member Author

Sorry for the late reply, I refactored the logic as discussed, please take a look again.

@siyuanfoundation
Copy link
Contributor

/approve

@jmhbnz for double check

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

This is a great first iteration, thanks @wzshiming

Can you please rebase following #34 which fixed some lint errors so we can merge.

@wzshiming wzshiming force-pushed the ci/scheme branch 3 times, most recently from b860e32 to 33281df Compare April 3, 2024 02:06
@wzshiming
Copy link
Member Author

Done.

@jmhbnz jmhbnz merged commit 68ca465 into etcd-io:master Apr 3, 2024
3 checks passed
@wzshiming wzshiming deleted the ci/scheme branch April 3, 2024 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants