ISSUE-1231 Enable test environment which don't require importing cluster #1232

HeartSaVioR · 2018-02-28T14:12:04Z

Add "test environment" (ID -1) to environment by default
Add "readonly" field to namespace table
- default: false
When readonly is true, default value (localhost) of Storm configuration is loaded
- it enables such environment to run topology test (good to test), but no more

* Add "test environment" (ID -1) to environment by default * Add "readonly" field to namespace table * default: false * When readonly is true, default value (localhost) of Storm configuration is loaded * it enables such environment to run topology test (good to test), but no more Change-Id: I027ee81eea1fc1480d0fd287ec13ffa6cc61ec88

HeartSaVioR · 2018-02-28T14:14:13Z

@shahsank3t
I would need UI change to complete the feature: disable option (edit/remove) on environment when rollback field is set to true. Could I get help on this? Thanks in advance.

HeartSaVioR · 2018-02-28T14:16:29Z

@arunmahadevan
This enables fresh new SAM instance to run topology (with importing existing one) test without importing cluster and setting up environment. #1230 even open the possibility to add TCP source and sink for composing test app as well.

shahsank3t · 2018-02-28T15:12:25Z

@HeartSaVioR Yes sure, I'll help you with the UI changes. Will this rollback field be part of the same environment API? Couldn't find anything related to the field in this PR so just confirming.

HeartSaVioR · 2018-02-28T15:23:58Z

Sorry for confusion, the field name is “readonly”, not “rollback”.

…

On Thu, 1 Mar 2018 at 00:16 Sanket Shah ***@***.***> wrote: @HeartSaVioR <https://github.com/heartsavior> Yes sure, I'll help you with the UI changes. Will this rollback field be part of the same environment API? Couldn't find anything related to the field in this PR so just confirming. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1232 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQZvXp4GAsUU4fpwgnnJcHQLZwoaNmCks5tZWzegaJpZM4SWtw8> .

joylyn · 2018-03-01T08:41:08Z

@HeartSaVioR I have a couple of doubts as follows -

Is the default value for readonly needed to be provided by user when adding a new environment?
In addition to disabling the edit/remove options for readonly namespaces, the "share" option is also disabled for user irrespective of the permission. Is this alright?

HeartSaVioR · 2018-03-01T10:23:59Z

@joylyn
The intention of patch is inserting "Test Environment" from the start of SAM when it doesn't exist, and let users (in fact, mostly contributors of SAM) use the environment to test simple scenarios which even don't need Storm cluster (local cluster of Storm would sufficient). readonly field is to prevent users to modify the test environment.

Nope. It is just a flag indicating that it shouldn't be modified from user side.
The namespace should be seen from all of users. Btw, I'm not aware of "share" option for namespace. It should not be needed.

@arunmahadevan Could you help me figuring out that "Test Environment" will be seen from all users? Sadly I'm not clear of ACL entry, and I don't add any ACL entry for such environment.

@ALL
Btw, If name of flag readonly makes confusion, we could find alternatives like builtin, testing and so on. What do you think?

arunmahadevan · 2018-03-01T19:24:54Z

Could you help me figuring out that "Test Environment" will be seen from all users? Sadly I'm not clear of ACL entry, and I don't add any ACL entry for such environment.

Are we hiding this placeholder Enviroment (e.g. from the enviroment REST API) so that it does not show up in the UI ?

The ACL entries are enforced at the Resource classes and catalog service can query any entry. However I don't see any permission checks currently happening for TestRunResource. We should protect these resources using the ACLs defined for the topology. (like how we protect the topology components using the underlying topology permissions here).

arunmahadevan

Left a few comments.

Are we hiding the placeholder entry from the REST APIs ?

arunmahadevan · 2018-03-01T19:27:29Z

bootstrap/sql/mysql/v012__introduce_test_environment.sql

+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+ALTER TABLE `namespace` ADD `readonly` BOOLEAN NOT NULL DEFAULT false;


may be better to name the flag testing or internal

internal would be fine to me.

arunmahadevan · 2018-03-01T19:28:39Z

...main/java/com/hortonworks/streamline/streams/actions/container/TopologyActionsContainer.java

        Service streamingEngineService = getFirstOccurenceServiceForNamespace(namespace, streamingEngine);
        if (streamingEngineService == null) {
-            throw new RuntimeException("Streaming Engine " + streamingEngine + " is not associated to the namespace " +
-                    namespace.getName() + "(" + namespace.getId() + ")");
+            if (!namespace.getReadonly()) {


isTesting or isInternal based on previous comment.

HeartSaVioR · 2018-03-01T22:09:36Z

@arunmahadevan

Are we hiding the placeholder entry from the REST APIs ?

Nope. It is just guarded with assertion (flag check), and UI should hide the edit functionality on internal.
It doesn't seem to make sense to hide from environment tab and show it for creating/importing/cloning topology. Is it possible? If not, we should just expose it (at least for get/list) to end users.

* change the name of field `readonly` to `internal` Change-Id: I6267b0087d6031b391c5ccc0824a22989d2d37da

HeartSaVioR · 2018-03-01T22:41:45Z

@arunmahadevan changed the field name.

arunmahadevan · 2018-03-02T02:25:31Z

@HeartSaVioR , does it require the UI changes to go along with it or can this be merged first ?

HeartSaVioR · 2018-03-02T02:34:40Z

@arunmahadevan Better to have follow-up patch since backend will throw exception whereas it is editable in UI, but the follow-up patch can be done after merging. I think @joylyn is following up the issue.

arunmahadevan · 2018-03-02T02:38:32Z

Raised #1236 for UI

HeartSaVioR requested a review from arunmahadevan February 28, 2018 14:12

arunmahadevan suggested changes Mar 1, 2018

View reviewed changes

ISSUE-1231 Enable test environment which don't require importing cluster

64ef21a

* change the name of field `readonly` to `internal` Change-Id: I6267b0087d6031b391c5ccc0824a22989d2d37da

HeartSaVioR mentioned this pull request Mar 1, 2018

Enable test environment which don't require importing cluster #1231

Closed

arunmahadevan approved these changes Mar 2, 2018

View reviewed changes

arunmahadevan mentioned this pull request Mar 2, 2018

UI changes to enable test environment which don't require importing cluster #1236

Closed

arunmahadevan closed this in 57ddd8d Mar 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISSUE-1231 Enable test environment which don't require importing cluster #1232

ISSUE-1231 Enable test environment which don't require importing cluster #1232

HeartSaVioR commented Feb 28, 2018

HeartSaVioR commented Feb 28, 2018

HeartSaVioR commented Feb 28, 2018

shahsank3t commented Feb 28, 2018

HeartSaVioR commented Feb 28, 2018 via email

joylyn commented Mar 1, 2018

HeartSaVioR commented Mar 1, 2018 •

edited

Loading

arunmahadevan commented Mar 1, 2018

arunmahadevan left a comment

arunmahadevan Mar 1, 2018

HeartSaVioR Mar 1, 2018

arunmahadevan Mar 1, 2018

HeartSaVioR Mar 1, 2018

HeartSaVioR commented Mar 1, 2018 •

edited

Loading

HeartSaVioR commented Mar 1, 2018

arunmahadevan commented Mar 2, 2018

HeartSaVioR commented Mar 2, 2018 •

edited

Loading

arunmahadevan commented Mar 2, 2018

ISSUE-1231 Enable test environment which don't require importing cluster #1232

ISSUE-1231 Enable test environment which don't require importing cluster #1232

Conversation

HeartSaVioR commented Feb 28, 2018

HeartSaVioR commented Feb 28, 2018

HeartSaVioR commented Feb 28, 2018

shahsank3t commented Feb 28, 2018

HeartSaVioR commented Feb 28, 2018 via email

joylyn commented Mar 1, 2018

HeartSaVioR commented Mar 1, 2018 • edited Loading

arunmahadevan commented Mar 1, 2018

arunmahadevan left a comment

Choose a reason for hiding this comment

arunmahadevan Mar 1, 2018

Choose a reason for hiding this comment

HeartSaVioR Mar 1, 2018

Choose a reason for hiding this comment

arunmahadevan Mar 1, 2018

Choose a reason for hiding this comment

HeartSaVioR Mar 1, 2018

Choose a reason for hiding this comment

HeartSaVioR commented Mar 1, 2018 • edited Loading

HeartSaVioR commented Mar 1, 2018

arunmahadevan commented Mar 2, 2018

HeartSaVioR commented Mar 2, 2018 • edited Loading

arunmahadevan commented Mar 2, 2018

HeartSaVioR commented Mar 1, 2018 •

edited

Loading

HeartSaVioR commented Mar 1, 2018 •

edited

Loading

HeartSaVioR commented Mar 2, 2018 •

edited

Loading