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

Before branching day: step 1 #2815

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1 @@
rpm-deps-mirroring-services
14 changes: 14 additions & 0 deletions cmd/branchingconfigmanagers/rpm-deps-mirroring-services/README.md
@@ -0,0 +1,14 @@
# RPM dependencies mirroring services
This manager attempts to automatize step (1.) of "[Few weeks before branching day](https://docs.google.com/document/d/1Z6ejnDCOCvNv9PWkyNPzVbjuLbDMAAT5GEeDpzb0SMs/edit#heading=h.r9xn02r1cyfn)" phase.

## Usage
### Options:
- `--current-release` specifies the current OCP version
- `--release-repo` is the absolution path to `openshift/release` repository

### Example
```sh
$ ./generated-release-gating-jobs \
--current-release "4.12" \
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought about it and it actually can stay that way. This tool is supporting only one action so there is absolutely no need for it to parse (outdated) lifecycle file

--release-repo "/full/path/to/openshift/release/repo"
```
86 changes: 86 additions & 0 deletions cmd/branchingconfigmanagers/rpm-deps-mirroring-services/main.go
@@ -0,0 +1,86 @@
package main

import (
"errors"
"flag"
"fmt"
"path"
"time"

"github.com/sirupsen/logrus"
"gopkg.in/ini.v1"

utilerrors "k8s.io/apimachinery/pkg/util/errors"

"github.com/openshift/ci-tools/pkg/api/ocplifecycle"
"github.com/openshift/ci-tools/pkg/branchcuts/bumper"
)

const (
rpmMirroringServicesPath = "core-services/release-controller/_repos"
rpmMirroringServicesGlobPatternFormat = "ocp-%s*.repo"
)

type options struct {
curOCPVersion string
releaseRepoDir string
logLevel int
}

func gatherOptions() (*options, error) {
var errs []error
o := &options{}
flag.StringVar(&o.curOCPVersion, "current-release", "", "Current OCP version")
flag.StringVar(&o.releaseRepoDir, "release-repo", "", "Path to 'openshift/release/ folder")
flag.IntVar(&o.logLevel, "log-level", int(logrus.DebugLevel), "Log level")
flag.Parse()

if _, err := ocplifecycle.ParseMajorMinor(o.curOCPVersion); o.curOCPVersion != "" && err != nil {
errs = append(errs, fmt.Errorf("error parsing cur-ocp-ver %s", o.curOCPVersion))
}

if o.releaseRepoDir != "" {
if !path.IsAbs(o.releaseRepoDir) {
errs = append(errs, errors.New("error parsing release repo path: path has to be absolute"))
}
} else {
errs = append(errs, errors.New("error parsing release repo path: path is mandatory"))
}

return o, utilerrors.NewAggregate(errs)
}

func main() {
o, err := gatherOptions()
if err != nil {
logrus.WithError(err).Fatal("failed to gather options")
}

logrus.SetLevel(logrus.Level(o.logLevel))
logrus.Debugf("using options %+v", o)

if err = reconcile(time.Now(), o); err != nil {
logrus.WithError(err).Fatal("failed to reconcile the status")
}
logrus.Info("status reconciled")
}

func reconcile(now time.Time, o *options) error {
logrus.Debugf("using options %+v", o)
bumpOpts := bumper.RepoBumperOptions{
FilesDir: path.Join(o.releaseRepoDir, rpmMirroringServicesPath),
GlobPattern: fmt.Sprintf(rpmMirroringServicesGlobPatternFormat, o.curOCPVersion),
CurOCPRelease: o.curOCPVersion,
}
logrus.Debugf("bumpOpts: %+v", bumpOpts)

b, err := bumper.NewRepoBumper(&bumpOpts)
if err != nil {
return fmt.Errorf("new repo bumper: %w", err)
}

if err := bumper.Bump[*ini.File](b, &bumper.BumpingOptions{}); err != nil {
return fmt.Errorf("bumper: %w", err)
}
return nil
}
2 changes: 1 addition & 1 deletion go.mod
Expand Up @@ -194,7 +194,7 @@ require (
google.golang.org/grpc v1.47.0 // indirect
google.golang.org/protobuf v1.28.0 // indirect
gopkg.in/inf.v0 v0.9.1 // indirect
gopkg.in/ini.v1 v1.63.2 // indirect
gopkg.in/ini.v1 v1.63.2
gopkg.in/square/go-jose.v2 v2.3.1 // indirect
gopkg.in/yaml.v3 v3.0.1
k8s.io/apiextensions-apiserver v0.24.2 // indirect
Expand Down
110 changes: 110 additions & 0 deletions pkg/branchcuts/bumper/bumper.go
@@ -0,0 +1,110 @@
package bumper
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these bumper files used in the second PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's why I have created this abstraction. It will become more obvious when you will be reviewing the second and third PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are correct. I know those tools could potentially miss some other sections as well, especially when it comes to handle non-structured text. As long as the field that it is going to be modified is well defined and it has a structure it is quite easy to perform the replacement. In all other cases I simply do not have strategies other than a naive text/regex replacement and that is dangerous because you could end up modifying text that it is not supposed to be modified.

Example:
Suppose you want to replace release-4.13 with release-4.14 using the sed expression s/4\.13/4\.14/g inside the following script:

curl -L https://github.com/openshift-cnv/cnv-ci/tarball/release-4.13 -o /tmp/cnv-ci.tgz
# Do some work here
# ...
# I'm going to do this and that because of a bug since version 4.13
# ...

That would be transformed into:

- curl -L https://github.com/openshift-cnv/cnv-ci/tarball/release-4.13 -o /tmp/cnv-ci.tgz
+ curl -L https://github.com/openshift-cnv/cnv-ci/tarball/release-4.14 -o /tmp/cnv-ci.tgz 
# Do some work here
# ...
- # I'm going to do this and that because of a bug since version 4.13
+ # I'm going to do this and that because of a bug since version 4.14
# ...

While the first replacement is probably good, the second one for sure it is not as we have changed the semantic of the original text.
Of course we could decide to set some rules as "skip comments if it is a bash script" but I have realized you won't never be 100% sure you made the correct decision if we go down this path.
I bet this is a problem that even @petr-muller faced again and again.
I am obviously open to any advises.

Copy link
Member

Choose a reason for hiding this comment

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

TBH I never worried too much about substitutions like this (which likely resulted in some wrong results but we were always on that fine line between process efficiency and correctness). I think the only place where we did sed substitution was bumping relesae payload jobs, and that was a basically a black box incantation that nobody eventually understood after a few bumps :)

That said, I think a reasonable approach is to not have ambition to bump versions in unstructured texts. You could have a contract that some part of the CI system exposes the "bumped" values in e.g. an envvar, and if people's payloads want to be handled by the tooling, they should use the nevvar. If they don't, they won't be bumped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and I agree. You should either follow the "contract" we established beforehand or those files (or at least some of their stanzas/sections/fields/whatever) won't be bumped to the next version.
That's it, unless we don't want to start training a neural network... that won't be 100% accurate either! :)

Copy link
Contributor

@jmguzik jmguzik Nov 25, 2022

Choose a reason for hiding this comment

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

+ # I'm going to do this and that because of a bug since version 4.14

Most of these problems will around curl as I noticed in the past so I would definitely welcome some regexp that is taking care at least of this case.

which likely resulted in some wrong results but we were always on that fine line between process efficiency and correctness

Generally, I would say yes and agree, but on the other hand, as we are not that experienced still, we would like to avoid possible situations when something is not working because we put the accent on efficiency instead of correctness. I also feel now more eyes are watching the process and there is a bigger pressure to do it right.

@danilo-gemoli with that being said, I am ok to merge it as an initial version that could be corrected in the future.


import (
"errors"
"fmt"
"os"
"path"

"github.com/sirupsen/logrus"

"github.com/openshift/ci-tools/pkg/util"
)

type Bumper[T any] interface {
GetFiles() ([]string, error)

Unmarshall(file string) (T, error)

BumpFilename(filename string, obj T) (string, error)

BumpContent(obj T) (T, error)

Marshall(obj T, bumpedFilename, dir string) error
}

type BumpingOptions struct {
OutDir string
}

// Bump bumps files using the Bumpers b according to the BumpingOptions.
func Bump[T any](b Bumper[T], o *BumpingOptions) error {
filesCh := make(chan string)
produce := func() error {
defer close(filesCh)
files, err := b.GetFiles()
logrus.Debugf("files: %+v", files)
if err != nil {
return err
}
for _, f := range files {
filesCh <- f
}
return nil
}
errsChan := make(chan error)
map_ := func() error {
for f := range filesCh {
if err := BumpObject(b, f, o.OutDir); err != nil {
errsChan <- err
}
}
return nil
}
return util.ProduceMap(0, produce, map_, errsChan)
}

func BumpObject[T any](b Bumper[T], file, outDir string) error {
logrus.Infof("bumping config %s", file)

srcFileFullPath := file

obj, err := b.Unmarshall(srcFileFullPath)
if err != nil {
logrus.WithError(err).Errorf("failed to unmarshall file %s", srcFileFullPath)
return fmt.Errorf("unmarshall file %s: %w", file, err)
}

filename := path.Base(file)

logrus.Infof("bumping filename %s", filename)
bumpedFilename, err := b.BumpFilename(filename, obj)
if err != nil {
logrus.WithError(err).Errorf("error bumping file %s", bumpedFilename)
return fmt.Errorf("bump filename: %w", err)
}
logrus.Infof("bumped filename %s", bumpedFilename)

outDir = getOutDir(file, outDir)
logrus.Debugf("out dir: %s", outDir)
dstFileFullPath := path.Join(outDir, bumpedFilename)

if _, err := os.Stat(dstFileFullPath); err == nil {
logrus.WithError(err).Warnf("file %s already exists, skipping", dstFileFullPath)
return fmt.Errorf("file %s already exists", dstFileFullPath)
} else if !errors.Is(err, os.ErrNotExist) {
return fmt.Errorf("file exists: %w", err)
}

logrus.Infof("bumping obj")
bumpedObj, err := b.BumpContent(obj)
if err != nil {
logrus.WithError(err).Error("error bumping obj")
return fmt.Errorf("bump object: %w", err)
}

logrus.Infof("marshalling obj %s to %s", bumpedFilename, outDir)
if err := b.Marshall(bumpedObj, bumpedFilename, outDir); err != nil {
logrus.WithError(err).Error("error marshalling2 obj")
return fmt.Errorf("marshall obj: %w", err)
}

return nil
}

func getOutDir(file string, dir string) string {
if dir != "" {
return dir
}
return path.Dir(file)
}
139 changes: 139 additions & 0 deletions pkg/branchcuts/bumper/repo-bumper.go
@@ -0,0 +1,139 @@
package bumper

import (
"bytes"
"fmt"
"io/fs"
"io/ioutil"
"os"
"path"
"strings"

"gopkg.in/ini.v1"

"github.com/openshift/ci-tools/pkg/api/ocplifecycle"
)

type (
RepoBumper struct {
GlobPattern string
FilesDir string
OCPRelease ocplifecycle.MajorMinor
}

RepoBumperOptions struct {
GlobPattern string
FilesDir string
CurOCPRelease string
}

rhelRepo struct {
BaseUrl string `ini:"baseurl,omitempty"`
Enabled int `ini:"baseurl,omitempty"`
FailoverMethod bool `ini:"failovermethod,omitempty"`
GPGCheck int `ini:"gpgcheck,omitempty"`
GPGKey string `ini:"gpgkey,omitempty"`
Name string `ini:"name,omitempty"`
PasswordFile string `ini:"password_file,omitempty"`
SkipIfUnavailable bool `ini:"skip_if_unavailable,omitempty"`
SSLClientCert string `ini:"sslclientcert,omitempty"`
SSLClientKey string `ini:"sslclientkey,omitempty"`
SSLVerify bool `ini:"sslverify,omitempty"`
UsernameFile string `ini:"username_file,omitempty"`
}
)

var (
majorMinorSeparators = []string{".", "-"}

_ Bumper[*ini.File] = &RepoBumper{}
)

func NewRepoBumper(o *RepoBumperOptions) (*RepoBumper, error) {
mm, err := ocplifecycle.ParseMajorMinor(o.CurOCPRelease)
if err != nil {
return nil, fmt.Errorf("Error parsing %s", o.CurOCPRelease)
}
return &RepoBumper{
GlobPattern: o.GlobPattern,
FilesDir: o.FilesDir,
OCPRelease: *mm,
}, nil
}

func (b *RepoBumper) GetFiles() ([]string, error) {
dirFs := os.DirFS(b.FilesDir)
matches, err := fs.Glob(dirFs, b.GlobPattern)
if err != nil {
return nil, err
}
files := make([]string, 0, len(matches))
for _, f := range matches {
fileFullPath := path.Join(b.FilesDir, f)
files = append(files, fileFullPath)
}
return files, nil
}

func (b *RepoBumper) Unmarshall(file string) (*ini.File, error) {
return ini.Load(file)
}

func (b *RepoBumper) BumpFilename(filename string, _ *ini.File) (string, error) {
curRelease := fmt.Sprintf("%d.%d", b.OCPRelease.Major, b.OCPRelease.Minor)
futureRelease := fmt.Sprintf("%d.%d", b.OCPRelease.Major, b.OCPRelease.Minor+1)
return strings.ReplaceAll(filename, curRelease, futureRelease), nil
}

func (b *RepoBumper) BumpContent(file *ini.File) (*ini.File, error) {
for _, section := range file.Sections() {
repo := rhelRepo{}
if err := section.MapTo(&repo); err != nil {
return nil, err
}

for _, s := range majorMinorSeparators {
curRelease := fmt.Sprintf("%d%s%d", b.OCPRelease.Major, s, b.OCPRelease.Minor)
futureRelease := fmt.Sprintf("%d%s%d", b.OCPRelease.Major, s, b.OCPRelease.Minor+1)
repo.BaseUrl = strings.ReplaceAll(repo.BaseUrl, curRelease, futureRelease)
}

if err := section.ReflectFrom(&repo); err != nil {
return nil, err
}
}
return file, nil
}

func (b *RepoBumper) Marshall(file *ini.File, bumpedFilename, dir string) error {
filePath := path.Join(dir, bumpedFilename)
return saveIniFile(filePath, file)
}

func saveIniFile(path string, f *ini.File) error {
ini.PrettySection = true
ini.PrettyFormat = false
ini.PrettyEqual = true

// What follow should have be avoided by using f.SaveTo(path) directly,
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not 100% get the explanation, also not sure if we should have such a detailed explanation here. What would be desired way to handle this issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The library prints an annoying double '\n' at the end of the file. I fixed this little glitch some months ago: go-ini/ini/pull/321 but we are still referencing the old version.

// but unfortunately it appends a double '\n' at the end of the file
// that makes it different from the original one: we should only bump the fields of
// interest without doing anything else.
// Consider opening a PR that fixes this issue, even if I'm not sure this can be
// considered an issue.
buf := bytes.NewBuffer(nil)
if _, err := f.WriteTo(buf); err != nil {
return err
}
bs := buf.Bytes()

doubleNewLine := ini.LineBreak + ini.LineBreak
if strings.HasSuffix(string(bs), doubleNewLine) {
bs = bs[0 : len(bs)-len(ini.LineBreak)]
}
if err := ioutil.WriteFile(path, bs, 0666); err != nil {
return err
}

return nil
}