Add config and README entry with regal new rule command

[Ronnie-personal]

Fixes #231

[Ronnie-personal]

@anderseknert
Is the code change overall heading to the right direction?
I would like understand what test cases should be added.

[anderseknert]

Looks good to me! I don't think the change in new.go belongs in this PR though, but should be in another one for that issue, right?

Also left a few comments, but mostly just details. Good work!

[anderseknert]

Oh wait, I was confused by the code from both of the PRs being included here, so I mistakenly commented on this as if it was the print-sprintf PR. Can you have all code relating to that PR removed from here so we can review this separately?

[Ronnie-personal]

Oh wait, I was confused by the code from both of the PRs being included here, so I mistakenly commented on this as if it was the print-sprintf PR. Can you have all code relating to that PR removed from here so we can review this separately?

Sorry about the confusion, I will remove the print-sprintf code change from this PR.

[Ronnie-personal]

@anderseknert

Could you please help me with a question? I'm trying to read data.ymal and unmarshal the file's content to config.Config type.

yamlContent, err := os.ReadFile("bundle/regal/config/provided/data.yaml") if err != nil { return err }

var existingConfig config.Config

// Unmarshal the YAML content into a map
if err := yaml.Unmarshal(yamlContent, &existingConfig); err != nil {
	return err
}`

There are no failures, however existingConfig only has 'level' field for each rule. Other attributes, such as 'max-file-length', are dropped.

Is there any other function I can call to retain all the fields of the rule?

[anderseknert]

@Ronnie-personal I'll take a look tonight 👍

[Ronnie-personal]

@Ronnie-personal I'll take a look tonight 👍

Thank you!

I also tried to unmarshal the yaml content to map[string]interface{} type, the rule's attributes are preserved, but the order and indent are messed up.

[anderseknert]

Could you please try and import gopkg.in/yaml.v3 instead of v2? I think our custom YAML marshalling/unmarshalling only works with v3 as that is the package we import in Regal. Perhaps we should add a public function in the config package which takes the path to a config and returns a Config struct? That way we at least make it somewhat easier to avoid this happening.

Once we have the unmarshalling right, we can look into the problems with marshalling.

[anderseknert]

I also noticed that "ignore" and "level" persist in extra attributes after being unmarshalled. This is fixed in #335, so make sure to rebase from main once that's been merged.

[Ronnie-personal]

Thank you, I will work on it tonight.

[Ronnie-personal]

Seems everything is working except the order of the rule's attributes, I guess the order change does not break anything else.

[Ronnie-personal]

Can I clarify the requirement? Do we need to add builtin rule only to the README.md and bundle/regal/config/provided/data.yaml? What about the custom type rule?

[anderseknert]

Yes, only for --type builtin. We shouldn't touch the user's .regal/config.yaml file, and they might not have one to begin with.

[anderseknert]

This is shaping up really nice! I've left some comments and suggestions, and it would probably be good if we had an e2e test, but that's not a hard requirement.

[anderseknert]

This assumes the command is run from the project root directory. I guess that's an OK assumption to make, but perhaps we should check for that and fail with a message saying that regal new rule --type builtin must be run from that location?

[anderseknert]

This should never be nil, as the provided configuration will always have rules, right?

[anderseknert]

This is clever! I wonder though, won't the "startPattern" break if we introduce a rule with a longer name than what we've previously had?

There's some code from before to render the table in the README over here: https://github.com/StyraInc/regal/blob/main/cmd/table.go

Do you think it would be worth refactoring that into a common function that we could reuse here?

[Ronnie-personal]

Thanks for the comments, shall I use function createTable instead?

I think recreate the entire table is simpler than adding the new rule delta.

I assume the function read all categories and rules from data.yaml and construct table. So I tried to use the function, but I don't understand line #91, when I debug the code, result.Modules appears to empty.

[Ronnie-personal]

This is shaping up really nice! I've left some comments and suggestions, and it would probably be good if we had an e2e test, but that's not a hard requirement.

Thanks for the review! It's a great idea to have an e2e test, I'm looking into it.
Here is the draft test code. I encountered error when call regal(). 'regal' runs from e2e sub-directory, but the func addToDataYAML(params newRuleCommandParams) expects the top level directory as the current working directory.

I would like to modify 'addToDataYAML' to use full path instead of relative path. I think we can obtain the current executable path from main.go (e.g os.Executable()) and pass along. I assume that it's mandatory to place 'regal' executable file in the top level directory of our repo.

`func TestCreateNewBuiltinRuleYamlReadme(t *testing.T) {
t.Parallel()

stdout := bytes.Buffer{}
stderr := bytes.Buffer{}

tmpDir := t.TempDir()

err := regal(&stdout, &stderr)("new", "rule", "--type", "builtin", "--category", "naming", "--name", "foo-bar-baz", "--output", tmpDir)

if exp, act := 0, ExitStatus(err); exp != act {
	t.Errorf("expected exit status %d, got %d", exp, act)
}

// Read the content of README.md
readmeContent, err := os.ReadFile("../README.md")
if err != nil {
	t.Fatalf("failed to read README.md: %v", err)
}

// Define an expected entry for the new rule
expectedEntry := "| naming    | [foo-bar-baz](https://docs.styra.com/regal/rules/naming/foo-bar-baz)                             | Place holder, description of the new rule"

// Check if the expected entry exists in README.md
if !strings.Contains(string(readmeContent), expectedEntry) {
	t.Errorf("expected entry not found in README.md:\n%s", expectedEntry)
}

// Read the content of data.yaml
yamlContent, err := os.ReadFile("../bundle/regal/config/provided/data.yaml")
if err != nil {
	t.Fatalf("failed to read data.yaml: %v", err)
}

var existingConfig config.Config

// Unmarshal the YAML content into a Config struct
if err := yaml.Unmarshal(yamlContent, &existingConfig); err != nil {
	t.Fatalf("failed to unmarshal data.yaml: %v", err)
}

// Check if the new rule exists in the YAML structure
if _, exists := existingConfig.Rules["naming"]["foo-bar-baz"]; !exists {
	t.Errorf("new rule not found in data.yaml")
}

}`

[anderseknert]

I assume that it's mandatory to place 'regal' executable file in the top level directory of our repo.

Yes, the e2e tests require that, and the build/do.rq e2e task will build the regal binary before running the tests.

[Ronnie-personal]

@anderseknert
I have made the code changes, can you please take another look when you have a chance?

Thanks.

[anderseknert]

Looks great! The only thing I'm not sure about is using os.Executable and setting the result to an EXECUTABLE_PATH env var. I have my Regal executable in ~/bin, and I would not want config/README to be stored there, but rather in the project root directory.

Could we have this logic updated to simply say:

"If we find bundle/regal/config/provided/data.yaml under the current working directory, we consider it a project root, and if we don't we exit with an error"?

Or is there something I forgot to consider? Other than that, this is good to merge. Well done! And thanks for being so patient :)

[Ronnie-personal]

Looks great! The only thing I'm not sure about is using os.Executable and setting the result to an EXECUTABLE_PATH env var. I have my Regal executable in ~/bin, and I would not want config/README to be stored there, but rather in the project root directory.

Could we have this logic updated to simply say:

"If we find bundle/regal/config/provided/data.yaml under the current working directory, we consider it a project root, and if we don't we exit with an error"?

Or is there something I forgot to consider? Other than that, this is good to merge. Well done! And thanks for being so patient :)

Thanks for the review, I will work on it tonight.

[anderseknert]

What's the status here @Ronnie-personal? I'm looking to cut a new release sometime early next week, and would love to have this included 🙂

[Ronnie-personal]

What's the status here @Ronnie-personal? I'm looking to cut a new release sometime early next week, and would love to have this included 🙂

@anderseknert Sorry about the late response, I'm done the code change, please review again when you have chance.

[anderseknert]

LGTM. Just not sure about the test. Also, please rebase from main. I think there's some change to the config file that's causing the conflict, but should be trivial to merge.

[anderseknert]

Not sure I understand this test :) Do we expect the command to exit with an exit code of 1?

[Ronnie-personal]

Good question and sorry about the confusion.
The working directory for e2e test is ./e2e.
The new.go expects the project top level folder as the current working directory, otherwise it will fail with error message "data.yaml file not found. Please run this command from the top-level directory of the Regal repository"

I once tried to change the e2e test's working directory, but the entire test stack is based on ./e2e directory, I'm not sure if I can do the change.
I also tried to use the full path in new.go instead of the relative path, but it requires to pass along the executable path.

[anderseknert]

Ah, I see. I think you can just go ahead and remove this test then.

[Ronnie-personal]

Sure, I'm doing it right now.

[Ronnie-personal]

LGTM. Just not sure about the test. Also, please rebase from main. I think there's some change to the config file that's causing the conflict, but should be trivial to merge.

I will take care of the rebase in one hour as soon as I logon to my personal desktop.

[Ronnie-personal]

@anderseknert
I have done the cleanup. Thanks!

[anderseknert]

Excellent! Thanks for sticking around 😃