SUSE / velum

Dashboard for CaaS Platform clusters (v1, v2 and v3)
https://www.suse.com/
Apache License 2.0
54 stars 30 forks source link

Add dex connector ldap table, models, and pillar output #601

Closed nanoscopic closed 6 years ago

nanoscopic commented 6 years ago

Added a new table "dex_connectors_ldap" and migration file to the db schema and migrate files. Altered pillar controller to output this new data. Added a basic/empty app model for the new table so that the data can be fetched through it. Updated pillar rspec to test newly output connectors as well as to allow the empty connectors in the other pillar tests.

nanoscopic commented 6 years ago

If there is a way to avoid merge of change to 34, I don't know how. Someone will need to explain to me.

Also, I don't understand what the contents of the patch file should be and how they are determined. It is clearly a diff patch between two files but I don't get what the one I am supposed to be diffing against is. The current head db/schema file?

MaximilianMeister commented 6 years ago

Also, I don't understand what the contents of the patch file should be and how they are determined. It is clearly a diff patch between two files but I don't get what the one I am supposed to be diffing against is. The current head db/schema file?

usually to generate the diff, you have to:

nanoscopic commented 6 years ago

Jenkins is now failing saying a manual rebase is required. Does it specifically forbid merge commits?

Sequence currently:

  1. I have my changes; that I have been repeatedly changing, amending, and pushing back up
  2. To get updates, I rebase on top of master
  3. 2# no longer works due to conflict
  4. I attempted to use a merge; that seems forbidden by jenkins
  5. I cherry picked my commit on top of master
  6. That causes a conflict to show in github
  7. I believe #6 is caused by continually commit amending without updating the date
  8. I commit amended and updated the date
  9. I attempt to push the changes upstream dex_connectors_2 branch ( this PR )
  10. Github ignores my push, with or without --force. It believes it has the current stuff. Changes to only date don't seem to be accepted.
  11. I attempted to change a single character to get github to accept it, then change back. Github still ignores pushing this to the dex_connectors_2 branch. I am notably able to push it to dex_connectors branch on github.
  12. I used git diff HEAD^ HEAD db/schema.rb to get a diff between the new schema file with ...34 and the previous one with ...33 ( without the ldap changes ). I just updated the index changes. This does not seem to be correct to me, because the index hashes, are, I believe, of the whole file. I am still unclear on what exact two versions of db/schema I should be diffing. If between previous commit and this one the contents of the file are much more different as they would show the new ldap changes also. Please clarify how to create this updated file with correct index values.
  13. Despite likely improperly updated the patch file, I attempted to push to dex_connectors_2 branch again. Still cannot. I can though, as on #11, push to dex_connectors branch and I have. I think the dex_connectors_2 branch is broken due to what I have done so far.
MaximilianMeister commented 6 years ago

@nanoscopic due to the fact that another bugfix got merged before this one there's a conflict now in the schema.rb

so the first thing to do is to rebase your branch to upstream/master to have the latest changes

# if you have your remote not set to upstream yet
git remote add upstream https://github.com/kubic-project/velum
# then get the newest references and rebase your local branch in one command
git pull --rebase upstream/master

this will mark db/schema.rb with the conflicts

 14 <<<<<<< HEAD
 15 ActiveRecord::Schema.define(version: 20181708070233) do
 16 =======
 17 ActiveRecord::Schema.define(version: 20181708070234) do
 18 >>>>>>> Add dex connector ldap table, models, and pillar output

to solve the conflict you just have to delete the line from HEAD which is the old state, and leave yours

 14 ActiveRecord::Schema.define(version: 20181708070234) do

now you just need to mark the conflict as resolved, and finish the rebase:

git add db/schema.rb
git rebase --continue

next step is to rebase 0_set_default_salt_events_after_time_column_value.rpm.patch

I usually open db/schema.rb and in this case go to line 110 and replace

110     t.datetime "alter_time",                    null: false

with

110     t.column   "alter_time",   "DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP"

and then go to line 139 and replace

139     t.datetime "alter_time",                  null: false

with

139     t.column   "alter_time", "DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP"

and then run

git diff db/schema.rb > packaging/suse/patches/0_set_default_salt_events_alter_time_column_value.rpm.patch

now the patch is rebased, and you can commit it

# first you need to clear the changes in db/schema.rb as we only used it to generate the patch
git checkout db/schema.rb
# then add and commit the patch
git add packaging/suse/patches/0_set_default_salt_events_alter_time_column_value.rpm.patch
git commit --amend
# and finally push it to your remote branch
git push --force [your remote] dex_connectors_2

the rebase of the patch is always necessary due to our packaging automation, which requires a strict patch which applies cleanly, otherwise we would always guess some higher fuzz factor when applying the patch during build time in open build service.

i hope this helped, if you want you can give me access to your branch and i can push the changes up to your branch directly, as i have done it already locally

nanoscopic commented 6 years ago

Following the steps you described worked great. Thank you for clearly showing the steps that work. It looks like what was tripping me up mostly was merging instead of rebasing master into my branch.

After doing as you describe I seem to have been able to cleanly update everything and push it back.

nanoscopic commented 6 years ago

Hmm. I see. Thank you for that small fix so that it can pass all CI. Hopefully we can fix the schema numbers to go back to dates somehow in the near future before the new year. It is yucky continuing on with the incrementing numbers.