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

Added migration identifier to all migration related logs #306

Merged
merged 2 commits into from
Sep 12, 2019

Conversation

Danil-Grigorev
Copy link
Contributor

@Danil-Grigorev Danil-Grigorev commented Sep 6, 2019

{"level":"info","ts":1567791963.0011566,"logger":"migration|z695n","msg":"[RUN]","migration":"28ea12a0-d0ce-11e9-9a4f-6399de957532","stage":true,"phase":"EnsureStagePods"}
{"level":"info","ts":1567791964.275831,"logger":"migration|z695n","msg":"Stage pod created.","migration":"28ea12a0-d0ce-11e9-9a4f-6399de957532","ns":"mssql-persistent","name":"mssql-deployment-1-zx8sv-stage"}
{"level":"error","ts":1567791964.5248604,"logger":"migration|z695n","msg":"","migration":"28ea12a0-d0ce-11e9-9a4f-6399de957532","error":"Pod \"mssql-deployment-1-zx8sv\" is invalid: spec: Forbidden: pod updates may not change fields other than `spec.containers[*].image`, `spec.initContainers[*].image`, `spec.activeDeadlineSeconds` or `spec.toleratio

Added migration field to simplify the selection of migration related log entries. This could help filtering logs in the UI.

@@ -118,8 +119,6 @@ type Task struct {
func (t *Task) Run() error {
t.Log.Info(
Copy link
Contributor

@jortel jortel Sep 6, 2019

Choose a reason for hiding this comment

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

This can probably go on (1) line now as well.

Copy link
Contributor

@jortel jortel left a comment

Choose a reason for hiding this comment

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

Seems reasonable.

@jortel
Copy link
Contributor

jortel commented Sep 6, 2019

@Danil-Grigorev I have a branch with some logger fixes: #308 and added SetValues() so it's not necessary to modify the Logger internals (Real). Please consider reviewing and basing your PR on this instead.

@jortel
Copy link
Contributor

jortel commented Sep 6, 2019

Looking at:

"Migration [RUN]","migration":"foo"

The word Migration seems kind of redundant with the migration keyword. Perhaps the logging in the Task should just be:

"RUN","migration":"foo"

I still like the all CAPS because it makes those entries easier to pick out.

Thoughts?

@Danil-Grigorev
Copy link
Contributor Author

I'll include all the changes on top of the #308. Agree with all suggestions.

Copy link
Contributor

@djwhatle djwhatle left a comment

Choose a reason for hiding this comment

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

MigMigration name alone (without namespace) isn't necessarily unique, but since we expect all MigMigrations to be created in the same namespace I think this will work well enough for log filtering purposes without adding excessive clutter to the logs.

@sseago
Copy link
Contributor

sseago commented Sep 10, 2019

We need to be careful assuming all migmigrations are in the same namespace. If it's just for logging purposes, it's probably OK, but nothing prevents CLI users from putting migrations in whatever namespaces they want to use. I've tested this and migrations work fine, even if created in a different namespace.

@jortel jortel merged commit b2e75d8 into migtools:master Sep 12, 2019
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.

4 participants