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

onboard b4mad-racing #1236

Merged
merged 2 commits into from
Oct 28, 2021
Merged

onboard b4mad-racing #1236

merged 2 commits into from
Oct 28, 2021

Conversation

durandom
Copy link
Member

@sesheta sesheta added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 28, 2021
@sesheta sesheta requested review from hemajv and larsks October 28, 2021 11:11
@sesheta sesheta added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 28, 2021
@durandom
Copy link
Member Author

[root@edeb1e1db75c home]# opfcli create-project b4mad-racing b4mad -d 'B4mad Racing'
INFO[0000] group b4mad already exists (continuing)
INFO[0000] rolebinding already exists (continuing)
INFO[0000] writing namespace definition to /home/cluster-scope/base/core/namespaces/b4mad-racing
[root@edeb1e1db75c home]# cd cluster-scope/overlays/prod/smaug
bash: cd: cluster-scope/overlays/prod/smaug: No such file or directory
[root@edeb1e1db75c home]# cd cluster-scope/overlays/prod/
[root@edeb1e1db75c prod]# ls
common  emea  moc  osc
[root@edeb1e1db75c prod]# cd moc/smaug/
[root@edeb1e1db75c smaug]# ls
configmaps  groups  ingresscontrollers  kustomization.yaml  nodenetworkconfigurationpolicies  oauths  secret-generator.yaml  secrets  storageclasses  subscriptions
[root@edeb1e1db75c smaug]# kustomize edit add resource ../../../../base/core/namespaces/
Display all 109 possibilities? (y or n)
[root@edeb1e1db75c smaug]# kustomize edit add resource ../../../../base/core/namespaces/b4mad-racing

@@ -1,136 +1,136 @@
---
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure why this touched all those lines.
see the cmds I've executed in the PR comments
they are copied from https://github.com/operate-first/hitchhikers-guide/blob/main/pages/onboarding_project.ipynb

Copy link
Member Author

Choose a reason for hiding this comment

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

@tumido @HumairAK is this because opfcli needs an update?

Copy link
Member

@HumairAK HumairAK Oct 28, 2021

Choose a reason for hiding this comment

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

edit: sorry misread

it's because you used kustomize cli, it reformated the file, just add the line:
../../../../base/core/namespaces/b4mad-racing
alphabetically and it should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

what's interesting is when I see this via the vscode github editor by pressing . on the pr, it doesn't show as massive of a diff.

name: racing
spec:
destination:
name: zero
Copy link
Member

Choose a reason for hiding this comment

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

this should be smaug

@@ -4,5 +4,6 @@ components:
- ../../../../components/project-view-public
kind: Kustomization
namespace: b4mad-minecraft
namespace: b4mad-racing
Copy link
Member

@HumairAK HumairAK Oct 28, 2021

Choose a reason for hiding this comment

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

This might be a typo but:
can't have more than one namespace in a kustomization.yaml
the files in b4mad-racing/ should be sufficient

- ingresscontrollers/default.yaml
- nodenetworkconfigurationpolicies/vlan-210-nfs.yaml
- nodenetworkconfigurationpolicies/vlan-211-nese.yaml
- ../../../../base/core/namespaces/b4mad-racing
Copy link
Member

Choose a reason for hiding this comment

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

can you add this alphabetically? we should update the cli to take this into account

@durandom
Copy link
Member Author

@HumairAK I've restarted the PR with just the documented commands (see commit messages)

[root@edeb1e1db75c smaug]# opfcli version
Name: opfcli
Version: v0.2.0
Git Commit Hash: a789dbd852
Build Date: 2021-07-07T14:56:34

@durandom
Copy link
Member Author

I can edit the files manually, but would we then need to create a bug issue in the opfcli repo?

@HumairAK
Copy link
Member

@durandom these commands:

configmaps  groups  ingresscontrollers  kustomization.yaml  nodenetworkconfigurationpolicies  oauths  secret-generator.yaml  secrets  storageclasses  subscriptions
[root@edeb1e1db75c smaug]# kustomize edit add resource ../../../../base/core/namespaces/
Display all 109 possibilities? (y or n)
[root@edeb1e1db75c smaug]# kustomize edit add resource ../../../../base/core/namespaces/b4mad-racing

will refactor the kustomization file alongside adding the entries, this part isn't handled by the opfcli yet so what I would suggest is to undo the changes to the kustomization in the smaug/ directory and manually add these files. We should update the notebook to not use the kustomize edit add command

@durandom
Copy link
Member Author

/unhold

@sesheta sesheta added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 28, 2021
@durandom durandom changed the title [WIP] onboard b4mad-racing onboard b4mad-racing Oct 28, 2021
@sesheta sesheta removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 28, 2021
Copy link
Member

@HumairAK HumairAK left a comment

Choose a reason for hiding this comment

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

/lgtm
/approved

@sesheta sesheta added the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2021
@HumairAK HumairAK merged commit 6824c4b into operate-first:master Oct 28, 2021
@durandom
Copy link
Member Author

fixes operate-first/support#350

@sesheta
Copy link
Member

sesheta commented Oct 28, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HumairAK

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sesheta sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants