)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"7ec324c0b1e3cdd6b4b9d420cbf61f66d024d521","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"5a69daf9_ef636724","updated":"2025-05-21 00:36:45.000000000","message":"some minor comments in line but over all this looks good.\n\ni guess we also should nable a minimal devstack job as part of this?\n\nthe ? is more we dont have any tests to run so while we can do a basic stack there really isnt much point in running tempst. it is useful to have a minimal job to test the devstack plugin. but not critical until we actuly have something more real.\n\n\nincase it help i basically put my working local.conf here \n\nhttps://review.opendev.org/c/openstack/grian-ui/+/950472/1/README.rst#143\n\nwhich is a varation fo the watcher conftoler local.conf \nhttps://github.com/openstack/watcher/blob/master/devstack/local.conf.controller\nwith wathcer turned off\n\n\nthe watcher-sg-core-tempest-base jobs \nhttps://github.com/openstack/watcher/blob/master/.zuul.yaml#L128\n\ncould be an ok basline to work from if we turn off watcher and repalce it with this plugin but we can start simple.\n\nif you want to defer the job to a follow up that is fine too.\nbut once we have something mostly functional and stable we shoudl enable it.","commit_id":"8e76c9d6ce8c047a4bb7505730da2f2d0a0154fd"},{"author":{"_account_id":6413,"name":"Victoria Martinez de la Cruz","email":"victoria@redhat.com","username":"vkmc"},"change_message_id":"052ef130f30b84b52c2fbd2cb6b9220fd39ef8fe","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"a62cfdc7_0db14449","in_reply_to":"5a69daf9_ef636724","updated":"2025-05-22 07:42:15.000000000","message":"Done","commit_id":"8e76c9d6ce8c047a4bb7505730da2f2d0a0154fd"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"4fd4997f23430a5c5574c95fe723b47cec4c2d5b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"f8771594_9b846192","updated":"2025-05-21 14:06:24.000000000","message":"lets proceed with this and we can refine as need in followup.\n\nonce this is mergd ill rebase my open patch and i can then provide an example local.conf in the devstack dir instead of embedding it in the readme.","commit_id":"af606c85a8faee36a172a03732a1efa1c805b86d"},{"author":{"_account_id":6413,"name":"Victoria Martinez de la Cruz","email":"victoria@redhat.com","username":"vkmc"},"change_message_id":"7cd83de3a7ebe26c7d1a61248f5ca0b1ef254f7b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"44bdce52_7173f340","in_reply_to":"f8771594_9b846192","updated":"2025-05-22 07:41:57.000000000","message":"All right, thanks!","commit_id":"af606c85a8faee36a172a03732a1efa1c805b86d"}],"devstack/plugin.sh":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"7ec324c0b1e3cdd6b4b9d420cbf61f66d024d521","unresolved":true,"context_lines":[{"line_number":4,"context_line":"    setup_develop ${GRIAN_UI_DIR}"},{"line_number":5,"context_line":"}"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"# check for services enabled"},{"line_number":8,"context_line":"if is_service_enabled horizon \u0026\u0026 \\"},{"line_number":9,"context_line":"   is_service_enabled ceilometer \u0026\u0026 \\"},{"line_number":10,"context_line":"   is_service_enabled sg-core \u0026\u0026 \\"},{"line_number":11,"context_line":"   is_service_enabled grian-ui; then"},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"    if [[ \"$1\" \u003d\u003d \"stack\" \u0026\u0026 \"$2\" \u003d\u003d \"pre-install\" ]]; then"},{"line_number":14,"context_line":"        # Set up system services"}],"source_content_type":"text/x-sh","patch_set":3,"id":"fcfd7f95_3501cc73","line":11,"range":{"start_line":7,"start_character":0,"end_line":11,"end_character":36},"updated":"2025-05-21 00:36:45.000000000","message":"so in general we do nto need to guard devstack plugins like this\n\nsg-core is hopefully not something we will need long term and it should be possible to deploy without ceilometer if you really wanted too as well\n\ni would be inclined to entirely remove this iff an de-dent it.","commit_id":"8e76c9d6ce8c047a4bb7505730da2f2d0a0154fd"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"fc63076cea8fefb9acb62dfd43e9f82b4c0dfba4","unresolved":true,"context_lines":[{"line_number":4,"context_line":"    setup_develop ${GRIAN_UI_DIR}"},{"line_number":5,"context_line":"}"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"# check for services enabled"},{"line_number":8,"context_line":"if is_service_enabled horizon \u0026\u0026 \\"},{"line_number":9,"context_line":"   is_service_enabled ceilometer \u0026\u0026 \\"},{"line_number":10,"context_line":"   is_service_enabled sg-core \u0026\u0026 \\"},{"line_number":11,"context_line":"   is_service_enabled grian-ui; then"},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"    if [[ \"$1\" \u003d\u003d \"stack\" \u0026\u0026 \"$2\" \u003d\u003d \"pre-install\" ]]; then"},{"line_number":14,"context_line":"        # Set up system services"}],"source_content_type":"text/x-sh","patch_set":3,"id":"dacf5c5c_1e7db991","line":11,"range":{"start_line":7,"start_character":0,"end_line":11,"end_character":36},"in_reply_to":"a33d3f57_039cbe16","updated":"2025-05-21 12:19:21.000000000","message":"we have metrics form node-exporter also so if ceimometer and sg-core are disabels we wills still host host metrics from node exporter.\n\nif we enabel the fake datasouce we also wont need it.\n\nso that why i dont want ot put the check in the plugin\n\nthe horizon one mease sense.","commit_id":"8e76c9d6ce8c047a4bb7505730da2f2d0a0154fd"},{"author":{"_account_id":6413,"name":"Victoria Martinez de la Cruz","email":"victoria@redhat.com","username":"vkmc"},"change_message_id":"e223bef8141a28a5a862b69aa2a1e22cd6469680","unresolved":false,"context_lines":[{"line_number":4,"context_line":"    setup_develop ${GRIAN_UI_DIR}"},{"line_number":5,"context_line":"}"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"# check for services enabled"},{"line_number":8,"context_line":"if is_service_enabled horizon \u0026\u0026 \\"},{"line_number":9,"context_line":"   is_service_enabled ceilometer \u0026\u0026 \\"},{"line_number":10,"context_line":"   is_service_enabled sg-core \u0026\u0026 \\"},{"line_number":11,"context_line":"   is_service_enabled grian-ui; then"},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"    if [[ \"$1\" \u003d\u003d \"stack\" \u0026\u0026 \"$2\" \u003d\u003d \"pre-install\" ]]; then"},{"line_number":14,"context_line":"        # Set up system services"}],"source_content_type":"text/x-sh","patch_set":3,"id":"b7ee2932_8bdfc683","line":11,"range":{"start_line":7,"start_character":0,"end_line":11,"end_character":36},"in_reply_to":"dacf5c5c_1e7db991","updated":"2025-05-21 13:21:25.000000000","message":"+1","commit_id":"8e76c9d6ce8c047a4bb7505730da2f2d0a0154fd"},{"author":{"_account_id":6413,"name":"Victoria Martinez de la Cruz","email":"victoria@redhat.com","username":"vkmc"},"change_message_id":"f7aed3d6fefd86d0db48e521e2a45f90c5b4ef9e","unresolved":false,"context_lines":[{"line_number":4,"context_line":"    setup_develop ${GRIAN_UI_DIR}"},{"line_number":5,"context_line":"}"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"# check for services enabled"},{"line_number":8,"context_line":"if is_service_enabled horizon \u0026\u0026 \\"},{"line_number":9,"context_line":"   is_service_enabled ceilometer \u0026\u0026 \\"},{"line_number":10,"context_line":"   is_service_enabled sg-core \u0026\u0026 \\"},{"line_number":11,"context_line":"   is_service_enabled grian-ui; then"},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"    if [[ \"$1\" \u003d\u003d \"stack\" \u0026\u0026 \"$2\" \u003d\u003d \"pre-install\" ]]; then"},{"line_number":14,"context_line":"        # Set up system services"}],"source_content_type":"text/x-sh","patch_set":3,"id":"a33d3f57_039cbe16","line":11,"range":{"start_line":7,"start_character":0,"end_line":11,"end_character":36},"in_reply_to":"fcfd7f95_3501cc73","updated":"2025-05-21 10:10:14.000000000","message":"IIRC horizon is enabled by default. I added the check, though, because since we require to move the enabled files to the horizon dir, if Horizon is explicitly disabled then that step will fail. I\u0027d rather fail earlier than get to the point in which we cannot enable the panels.\n\nRegarding ceilometer and sg-core, we would need those services to retrieve metrics. If we enable the option of using a different ceilometer backend, we can remove sg-core. But, for now, it will be the initial implementation. Unless I am missing something?\n\nAnd having the grian-ui check basically is for adding an explicit toggle for enabling or not this plugin.","commit_id":"8e76c9d6ce8c047a4bb7505730da2f2d0a0154fd"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"7ec324c0b1e3cdd6b4b9d420cbf61f66d024d521","unresolved":true,"context_lines":[{"line_number":21,"context_line":"    elif [[ \"$1\" \u003d\u003d \"stack\" \u0026\u0026 \"$2\" \u003d\u003d \"post-config\" ]]; then"},{"line_number":22,"context_line":"        # Configure after the other layer 1 and 2 services have been configured"},{"line_number":23,"context_line":"        echo_summary \"Configuring Grian UI\""},{"line_number":24,"context_line":"        cp -a ${GRIAN_UI_DIR}/grian_ui/local/enabled/* ${DEST}/horizon/openstack_dashboard/local/enabled/"},{"line_number":25,"context_line":"        cp -a ${GRIAN_UI_DIR}/grian_ui/local/local_settings.d/* ${DEST}/horizon/openstack_dashboard/local/local_settings.d/"},{"line_number":26,"context_line":"    elif [[ \"$1\" \u003d\u003d \"stack\" \u0026\u0026 \"$2\" \u003d\u003d \"extra\" ]]; then"},{"line_number":27,"context_line":"        # no-op"}],"source_content_type":"text/x-sh","patch_set":3,"id":"48ee7bdf_60b24461","line":24,"range":{"start_line":24,"start_character":14,"end_line":24,"end_character":54},"updated":"2025-05-21 00:36:45.000000000","message":"because the repo is using src layout you will need to do this instead\n\n\n```suggestion\n        cp -a ${GRIAN_UI_DIR}/src/grian_ui/local/enabled/* ${DEST}/horizon/openstack_dashboard/local/enabled/\n```\n\nhttps://packaging.python.org/en/latest/discussions/src-layout-vs-flat-layout/#src-layout-vs-flat-layout","commit_id":"8e76c9d6ce8c047a4bb7505730da2f2d0a0154fd"},{"author":{"_account_id":6413,"name":"Victoria Martinez de la Cruz","email":"victoria@redhat.com","username":"vkmc"},"change_message_id":"f7aed3d6fefd86d0db48e521e2a45f90c5b4ef9e","unresolved":false,"context_lines":[{"line_number":21,"context_line":"    elif [[ \"$1\" \u003d\u003d \"stack\" \u0026\u0026 \"$2\" \u003d\u003d \"post-config\" ]]; then"},{"line_number":22,"context_line":"        # Configure after the other layer 1 and 2 services have been configured"},{"line_number":23,"context_line":"        echo_summary \"Configuring Grian UI\""},{"line_number":24,"context_line":"        cp -a ${GRIAN_UI_DIR}/grian_ui/local/enabled/* ${DEST}/horizon/openstack_dashboard/local/enabled/"},{"line_number":25,"context_line":"        cp -a ${GRIAN_UI_DIR}/grian_ui/local/local_settings.d/* ${DEST}/horizon/openstack_dashboard/local/local_settings.d/"},{"line_number":26,"context_line":"    elif [[ \"$1\" \u003d\u003d \"stack\" \u0026\u0026 \"$2\" \u003d\u003d \"extra\" ]]; then"},{"line_number":27,"context_line":"        # no-op"}],"source_content_type":"text/x-sh","patch_set":3,"id":"db9f0b6c_0fc98dc9","line":24,"range":{"start_line":24,"start_character":14,"end_line":24,"end_character":54},"in_reply_to":"48ee7bdf_60b24461","updated":"2025-05-21 10:10:14.000000000","message":"+1","commit_id":"8e76c9d6ce8c047a4bb7505730da2f2d0a0154fd"}],"devstack/settings":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"7ec324c0b1e3cdd6b4b9d420cbf61f66d024d521","unresolved":true,"context_lines":[{"line_number":1,"context_line":"# settings file for grian-ui plugin"},{"line_number":2,"context_line":"enable_service grian-ui"},{"line_number":3,"context_line":""},{"line_number":4,"context_line":"# set up default directories"},{"line_number":5,"context_line":"GRIAN_UI_DIR\u003d${GRIAN_UI_DIR:\u003d$DEST/grian-ui}"}],"source_content_type":"application/octet-stream","patch_set":3,"id":"4961b885_f828707f","line":2,"range":{"start_line":2,"start_character":0,"end_line":2,"end_character":23},"updated":"2025-05-21 00:36:45.000000000","message":"i would generally consider this to be bad practice \nplugins should not enable the service they provide by default.","commit_id":"8e76c9d6ce8c047a4bb7505730da2f2d0a0154fd"},{"author":{"_account_id":6413,"name":"Victoria Martinez de la Cruz","email":"victoria@redhat.com","username":"vkmc"},"change_message_id":"6ffc616f0a17a26dcde92adfe72cd97e42f1089d","unresolved":true,"context_lines":[{"line_number":1,"context_line":"# settings file for grian-ui plugin"},{"line_number":2,"context_line":"enable_service grian-ui"},{"line_number":3,"context_line":""},{"line_number":4,"context_line":"# set up default directories"},{"line_number":5,"context_line":"GRIAN_UI_DIR\u003d${GRIAN_UI_DIR:\u003d$DEST/grian-ui}"}],"source_content_type":"application/octet-stream","patch_set":3,"id":"6398f998_9dcffc03","line":2,"range":{"start_line":2,"start_character":0,"end_line":2,"end_character":23},"in_reply_to":"4961b885_f828707f","updated":"2025-05-21 13:32:51.000000000","message":"Why you consider this a bad practice? From what I understand from here https://github.com/openstack/devstack/blob/9e81048bbb3b3adbfb7bd5307af9bce79290308c/functions-common#L2007, this simply includes this service in the list of enabled services that Devstack is aware of.","commit_id":"8e76c9d6ce8c047a4bb7505730da2f2d0a0154fd"},{"author":{"_account_id":6413,"name":"Victoria Martinez de la Cruz","email":"victoria@redhat.com","username":"vkmc"},"change_message_id":"7cd83de3a7ebe26c7d1a61248f5ca0b1ef254f7b","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# settings file for grian-ui plugin"},{"line_number":2,"context_line":"enable_service grian-ui"},{"line_number":3,"context_line":""},{"line_number":4,"context_line":"# set up default directories"},{"line_number":5,"context_line":"GRIAN_UI_DIR\u003d${GRIAN_UI_DIR:\u003d$DEST/grian-ui}"}],"source_content_type":"application/octet-stream","patch_set":3,"id":"18fed32a_d0d51464","line":2,"range":{"start_line":2,"start_character":0,"end_line":2,"end_character":23},"in_reply_to":"54f8fcbc_8a70d929","updated":"2025-05-22 07:41:57.000000000","message":"I see, thanks for clarifying. We can definitely revisit and remove this.","commit_id":"8e76c9d6ce8c047a4bb7505730da2f2d0a0154fd"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"4fd4997f23430a5c5574c95fe723b47cec4c2d5b","unresolved":true,"context_lines":[{"line_number":1,"context_line":"# settings file for grian-ui plugin"},{"line_number":2,"context_line":"enable_service grian-ui"},{"line_number":3,"context_line":""},{"line_number":4,"context_line":"# set up default directories"},{"line_number":5,"context_line":"GRIAN_UI_DIR\u003d${GRIAN_UI_DIR:\u003d$DEST/grian-ui}"}],"source_content_type":"application/octet-stream","patch_set":3,"id":"54f8fcbc_8a70d929","line":2,"range":{"start_line":2,"start_character":0,"end_line":2,"end_character":23},"in_reply_to":"6398f998_9dcffc03","updated":"2025-05-21 14:06:24.000000000","message":"because it deploys the service\n\nso adding the plugin via enabel plugin is not really ment to actuly make it do anything\n\nyou shoudl do the enable_service manually in the local.conf\n\nthis is less of an issue for the ui as it only has\none deliverable\n\nbut for any plugin that has more then one it can cause issuees espiclly if the plugin deploys some compoents on contoler and others on computes..\n\nits pretty conventional do enabel the service in the pugin but my experince is that generally end up makeing creating non trivial jobs harder over time.\n\ni.e. if a plugin adds a new service in a future release it breaks existing jobs if ti need extra configuration to work.\n\nAnyway I think we can proceed with this as is, but that\u0027s why i don\u0027t like that pattern in general.","commit_id":"8e76c9d6ce8c047a4bb7505730da2f2d0a0154fd"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"7ec324c0b1e3cdd6b4b9d420cbf61f66d024d521","unresolved":true,"context_lines":[{"line_number":2,"context_line":"enable_service grian-ui"},{"line_number":3,"context_line":""},{"line_number":4,"context_line":"# set up default directories"},{"line_number":5,"context_line":"GRIAN_UI_DIR\u003d${GRIAN_UI_DIR:\u003d$DEST/grian-ui}"}],"source_content_type":"application/octet-stream","patch_set":3,"id":"c231527e_e63340b4","line":5,"range":{"start_line":5,"start_character":29,"end_line":5,"end_character":43},"updated":"2025-05-21 00:36:45.000000000","message":"this either need to be $DEST/grian-ui/src\n\nor we shoudl adjust the paths elsewhere\n\n\ni also tend to add somthing like this \n\n```\nGRIAN_UI_DIR\u003d$( cd \"$( dirname \"${BASH_SOURCE[0]}\" )\" \u0026\u0026 cd .. \u0026\u0026 pwd )\n```\nthis will automaticlly find the root of the git repo relitive to this file.\n\nso if its  not under $DEST or has a diffent name it will still work.","commit_id":"8e76c9d6ce8c047a4bb7505730da2f2d0a0154fd"},{"author":{"_account_id":6413,"name":"Victoria Martinez de la Cruz","email":"victoria@redhat.com","username":"vkmc"},"change_message_id":"4a10e49f5963f81346ea9b792c93fd6b1eeca548","unresolved":false,"context_lines":[{"line_number":2,"context_line":"enable_service grian-ui"},{"line_number":3,"context_line":""},{"line_number":4,"context_line":"# set up default directories"},{"line_number":5,"context_line":"GRIAN_UI_DIR\u003d${GRIAN_UI_DIR:\u003d$DEST/grian-ui}"}],"source_content_type":"application/octet-stream","patch_set":3,"id":"5ed78527_b7876c42","line":5,"range":{"start_line":5,"start_character":29,"end_line":5,"end_character":43},"in_reply_to":"c231527e_e63340b4","updated":"2025-05-21 13:44:04.000000000","message":"For how we use GRIAN_UI_DIR env var, I think this can stay as is. In plugin.sh there are three uses of this var, one for the setup_develop (setup.py is located under $DEST/grian-ui so it doesn\u0027t require the src/ prefix), and mv the enabled files (in which case I already apprended src/)\n\nAlso, this env var can be overriden in the local.conf with the desired location as part of the Devstack setup.","commit_id":"8e76c9d6ce8c047a4bb7505730da2f2d0a0154fd"}]}